[jira] Commented: (DERBY-2379) provide encryption support for temporary files used by lob if the data base is encrypted

["Knut Anders Hatlen (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/DERBY-2379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12495643 ] 

Knut Anders Hatlen commented on DERBY-2379:
-------------------------------------------

Thanks for posting the new patch, Anurag! See my comments below.

> I haven't extended the LOBFile class from StorageRandomAccessFile as
> doing so will need empty implimentation of several methods
> (inherited from parent interfaces of StorageRandomAccessFile)

The implementations of StorageRandomAccessFile normally extend
java.io.RandomAccessFile which implements all the methods of the
StorageRandomAccessFile interface. Could that help LOBFile as well?

Some of my previous comments were not addressed or only partly
addressed. I have included them below, some of them with extra
comments. If you don't agree with my comments, please say so. There
are so many different cases that need to be covered, so I can very
well have misunderstood something.

> * LOBFile.getBlocks:
>   Is it necessary to check whether len is negative and thrown an
>   IndexOutOfBoundsException? I guess the boundaries are checked on a
>   higher level as well, and you'll get a NegativeArraySizeException
>   anyway if len in negative, so I don't see any need to explicitly
>   throw IndexOutOfBoundsException.

Not addressed as far as I can see.

> * The read*/write* methods of LOBFile have many calls to
>   RandomAccessFile.length(). Would it be possible to reduce the number
>   of times it is called? I'm not sure, but I suspect length() might
>   involve a system call, so one shouldn't call it too frequently. I
>   was thinking perhaps we could change code that looks like this
> + if (currentPos >= randomAccessFile.length()) {
> + //current postion is in memory
> + int pos = (int) (currentPos - randomAccessFile.length());
> into something like this:
>     long fileLength = randomAccessFile.length();
>     if (currentPos >= fileLength) {
>         int pos = (int) (currentPos - fileLength);

Partly addressed. Still occurrences of multiple calls to length() in
writeEncrypted(byte), writeEncrypted(byte[],int,int) and setLength().

> * LOBFile.writeEncrypted(byte[],int,int):
> + //copy the bytes from tail which won't be overwritten'
> + System.arraycopy (tail, 0, clearText, 0, pos);
> Shouldn't the last argument have been tailLength in case we are not
> overwriting the end of the tail?

Not addressed, but I don't understand what I meant by that comment, so
it's probably OK... ;)

> * LOBFile.readEncrypted(byte[],int,int) / writeEncrypted(byte[],int,int):
>   It's probably slightly faster to replace this kind of loops
> + for (int i = 0; i < cypherText.length / blockSize; i++) {
> + df.decrypt (cypherText, i * blockSize, blockSize, tmpByte,
> + i * blockSize);
> + }
>   with
>     for (int offset = 0; offset < cypherText.length; offset += blockSize) {
>         df.decrypt(cypherText, offset, blockSize, tmpByte, offset);
>     }

Not addressed, I think.

> * LOBFile.readEncrypted(byte[],int,int):
>   This part doesn't look correct to me:
> + //find out total number of bytes we can read
> + int newLen = (len - cypherText.length < tailSize)
> + ? len - cypherText.length
> + : tailSize;
>   I think it only works if the read starts on a block boundary, and it
>   should have used overflow instead of (len - cypherText.length).

Only partly addressed. I think both occurrences of (len -
cypherText.length) should have been replaced. Actually, I think it
could be as simple as newLen = min(overFlow, tailSize).

> * LOBFile.setLength() is a no-op if the new length is greater than the
>   old length. Wouldn't it be better to throw an error in this case?
>   It's not supposed to happen since it's only called from
>   LOBStreamControl.truncate(), but since setLength() is already
>   checking that the new size is not greater, I think it's better to
>   throw an IllegalArgumentException or UnsupportedOperationException
>   than to silently return.

This seems to have been fixed, but the javadoc still says that it's a
no-op.

A couple of other things I noticed when I looked at the patch again:

- The javadoc comments often just have a @param tag or @throws tag
  which tells the name of the parameter or exception, and doesn't
  describe it. It would be good if they also contained a description.

- Some of the @throws tags are on this form:
    @throws IOException, SQLException, StandardException
  Here, "SQLException, StandardException" will be interpreted as the
  description of IOException. Please use one @throws tag per
  exception.

- The javadoc for LOBFile.seek() says that it's a no-op if pos is
  greater than length. However, it will always modify currentPos, so
  it's not actually a no-op.

- LOBFile.readEncrypted(byte[],int,int) has this code:
+        if (newLen == 0)
+            return -1;
  This means -1 is returned if someone requests 0 bytes. I think the
  condition should also include "&& len > 0".

- When readEncrypted() and writeEncrypted() calculate the value of
  overFlow, I think it would be more readable if it were written as:
    int overFlow = (int) Math.max(0L, currentPos + len - fileLength);

> provide encryption support for temporary files used by lob if the data base is encrypted
> ----------------------------------------------------------------------------------------
>
>                 Key: DERBY-2379
>                 URL: https://issues.apache.org/jira/browse/DERBY-2379
>             Project: Derby
>          Issue Type: Sub-task
>          Components: JDBC
>    Affects Versions: 10.3.0.0
>         Environment: all
>            Reporter: Anurag Shekhar
>         Assigned To: Anurag Shekhar
>         Attachments: CosmeticComments_1.txt, derby-2379.diff, derby-2379v2.diff
>
>


-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.