Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: JDBC
    • Labels:
      None
    • Environment:
      all
    1. derby-2379-followup.diff
      3 kB
      Anurag Shekhar
    2. derby-2379v4.diff
      39 kB
      Anurag Shekhar
    3. derby-2379v4.diff
      39 kB
      Anurag Shekhar
    4. derby-2379v3.diff
      36 kB
      Anurag Shekhar
    5. derby-2379v2-conflict.diff
      36 kB
      Anurag Shekhar
    6. derby-2379v2.diff
      37 kB
      Anurag Shekhar
    7. CosmeticComments_1.txt
      2 kB
      V.Narayanan
    8. derby-2379.diff
      25 kB
      Anurag Shekhar

      Issue Links

        Activity

        Hide
        Anurag Shekhar added a comment -

        Description of patch derby-2379.diff
        This patch introduces a new class LOBFile which is encryption aware and uses store factory methods to encrypt and decrypt bytes before writing or after reading from the file in case of encrypted databse. This class is used by LOBStreamControl for accessing temporary file.
        It also contains changes in TestConfiguration and JDBCDataSource to enable support for running test suites in encrypted mode (only in embedded mode).

        New Files added
        java/engine/org/apache/derby/impl/jdbc/LOBFile.java
        Class to handle plain/encrypted io to temporary file.

        Modified Files
        java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java
        Modified to use LOBFile

        java/engine/org/apache/derby/iapi/store/raw/data/DataFactory.java
        added a new method to return true if the database is encrypted

        java/testing/org/apache/derbyTesting/junit/TestConfiguration.java
        java/testing/org/apache/derbyTesting/junit/JDBCDataSource.java
        Modified to run tests in encrypted mode.

        java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobSetMethodsTest.java
        java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/LobStreamTest.java

        Enabled in encrypted mode too.

        Show
        Anurag Shekhar added a comment - Description of patch derby-2379.diff This patch introduces a new class LOBFile which is encryption aware and uses store factory methods to encrypt and decrypt bytes before writing or after reading from the file in case of encrypted databse. This class is used by LOBStreamControl for accessing temporary file. It also contains changes in TestConfiguration and JDBCDataSource to enable support for running test suites in encrypted mode (only in embedded mode). New Files added java/engine/org/apache/derby/impl/jdbc/LOBFile.java Class to handle plain/encrypted io to temporary file. Modified Files java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java Modified to use LOBFile java/engine/org/apache/derby/iapi/store/raw/data/DataFactory.java added a new method to return true if the database is encrypted java/testing/org/apache/derbyTesting/junit/TestConfiguration.java java/testing/org/apache/derbyTesting/junit/JDBCDataSource.java Modified to run tests in encrypted mode. java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/BlobSetMethodsTest.java java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/LobStreamTest.java Enabled in encrypted mode too.
        Hide
        Mike Matrigali added a comment -

        Can you document a little about the intended usage of LOBFile? Ie. is it a purely a server side action vs. a client side? I
        am trying to understand if the store module usage from the jdbc layer is appropriate.

        Show
        Mike Matrigali added a comment - Can you document a little about the intended usage of LOBFile? Ie. is it a purely a server side action vs. a client side? I am trying to understand if the store module usage from the jdbc layer is appropriate.
        Hide
        Anurag Shekhar added a comment -

        LOBFile is a server side class and will be used by embedded jdbc driver.

        Show
        Anurag Shekhar added a comment - LOBFile is a server side class and will be used by embedded jdbc driver.
        Hide
        Knut Anders Hatlen added a comment -
        • It seems to me LOBFile implements all the methods of the
          StorageRandomAccessFile interface, but the class doesn't implement
          the interface. Would it make sense to let LOBFile implement
          StorageRandomAccessFile? That would require fewer changes in
          LOBStreamControl, and one could also leave the non-encrypted
          read/write methods out of LOBFile.
        • It would be good to add some comments explaining the purpose of the
          instance variables and the private methods in LOBFile.
        • Is the lobFile field in LOBFile necessary? It doesn't seem to be
          used outside the constructor.
        • 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.
        • LOBFile.getBlocks:
          Couldn't the calculation of endPos be expressed without the
          condition, like this:
          long endPos = (pos + len + blockSize - 1) / blockSize * blockSize;
          I'm not saying it will improve the readability significantly,
          though...
        • LOBFile.writeEncrypted(byte):
          tailSize = (pos > tailSize - 1) ? pos + 1 : tailSize;
          Perhaps this is clearer when written like this:
          if (pos >= tailSize) tailSize = pos + 1;
        • LOBFile.writeEncrypted(byte):
          + long l = randomAccessFile.length();
          + tailSize = 0;
          + }
          The variable l is never used.
        • 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);
        • LOBFile.writeEncrypted(byte[],int,int):
          + if (finalPos < blockSize) { + //updated size won't be enough to perform encryption' + System.arraycopy (b, off, tail, pos, len); + tailSize = pos + len; + currentPos += len; + return; + }

          Shouldn't tailSize be max(tailSize, pos+len)?

        and

        + //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?

        • LOBFile.readEncrypted():
          It doesn't seem like currentPos is incremented in the case where we
          need to read from the file.
        • 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); }
        • LOBFile.readEncrypted(byte[],int,int):
          currentPos is not incremented when overFlow == 0.
        • 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).
        • I don't understand why getFilePointer, seek, read(byte[],off,len)
          and setLength are synchronized. Could you explain why these methods
          need synchronization, and why the other methods don't?
        • 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.
        • I got these warnings when I tried to generate the javadoc:
          [javadoc] .../LOBFile.java:254: warning - @return tag has no arguments.
          [javadoc] .../LOBFile.java:346: warning - @return tag has no arguments.
          [javadoc] .../DataFactory.java:381: warning - @return tag has no arguments.
        • If my comments have revealed bugs in the patch, I think it would be
          good to write test cases which exposes them and add them to the
          JUnit tests.
        Show
        Knut Anders Hatlen added a comment - It seems to me LOBFile implements all the methods of the StorageRandomAccessFile interface, but the class doesn't implement the interface. Would it make sense to let LOBFile implement StorageRandomAccessFile? That would require fewer changes in LOBStreamControl, and one could also leave the non-encrypted read/write methods out of LOBFile. It would be good to add some comments explaining the purpose of the instance variables and the private methods in LOBFile. Is the lobFile field in LOBFile necessary? It doesn't seem to be used outside the constructor. 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. LOBFile.getBlocks: Couldn't the calculation of endPos be expressed without the condition, like this: long endPos = (pos + len + blockSize - 1) / blockSize * blockSize; I'm not saying it will improve the readability significantly, though... LOBFile.writeEncrypted(byte): tailSize = (pos > tailSize - 1) ? pos + 1 : tailSize; Perhaps this is clearer when written like this: if (pos >= tailSize) tailSize = pos + 1; LOBFile.writeEncrypted(byte): + long l = randomAccessFile.length(); + tailSize = 0; + } The variable l is never used. 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); LOBFile.writeEncrypted(byte[],int,int): + if (finalPos < blockSize) { + //updated size won't be enough to perform encryption' + System.arraycopy (b, off, tail, pos, len); + tailSize = pos + len; + currentPos += len; + return; + } Shouldn't tailSize be max(tailSize, pos+len)? and + //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? LOBFile.readEncrypted(): It doesn't seem like currentPos is incremented in the case where we need to read from the file. 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); } LOBFile.readEncrypted(byte[],int,int): currentPos is not incremented when overFlow == 0. 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). I don't understand why getFilePointer, seek, read(byte[],off,len) and setLength are synchronized. Could you explain why these methods need synchronization, and why the other methods don't? 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. I got these warnings when I tried to generate the javadoc: [javadoc] .../LOBFile.java:254: warning - @return tag has no arguments. [javadoc] .../LOBFile.java:346: warning - @return tag has no arguments. [javadoc] .../DataFactory.java:381: warning - @return tag has no arguments. If my comments have revealed bugs in the patch, I think it would be good to write test cases which exposes them and add them to the JUnit tests.
        Hide
        V.Narayanan added a comment -

        I was reviewing this patch yesterday and had a few comments too. I have not checked to see if the intersection of the set of comments of knut and that of mine is a null set but I am putting it out there anyway.

        My comments are mostly cosmetic except for the below. The attachment contains the cosmetic issues that I identified. I was planning to send the attachment internally only.

        you are doing the below in BlobSetMethods test. Shouldn't one of them have been
        embeddedEncryptedSuite instead of embeddedSuite?

        + TestSuite blobSuite = new TestSuite ("blob set tests");
        + blobSuite.addTest (TestConfiguration.embeddedSuite
        + (BlobSetMethodsTest.class));
        + blobSuite.addTest (TestConfiguration.embeddedSuite
        + (BlobSetMethodsTest.class));
        + return blobSuite;

        There is a similar issue in LobStreamTest.

        Also you have made setLength and read synchronized but readByte and all the write methods are not synchronized. Probably I am having this doubt because of my not knowing the code properly but can you explain why you have done this?

        The cosmetic comments I had can be ignored if you want to. I consider them trivial. For whatever it is worth I wrote those comments in the form of a text file and am attaching the file to this issue.

        Show
        V.Narayanan added a comment - I was reviewing this patch yesterday and had a few comments too. I have not checked to see if the intersection of the set of comments of knut and that of mine is a null set but I am putting it out there anyway. My comments are mostly cosmetic except for the below. The attachment contains the cosmetic issues that I identified. I was planning to send the attachment internally only. you are doing the below in BlobSetMethods test. Shouldn't one of them have been embeddedEncryptedSuite instead of embeddedSuite? + TestSuite blobSuite = new TestSuite ("blob set tests"); + blobSuite.addTest (TestConfiguration.embeddedSuite + (BlobSetMethodsTest.class)); + blobSuite.addTest (TestConfiguration.embeddedSuite + (BlobSetMethodsTest.class)); + return blobSuite; There is a similar issue in LobStreamTest. Also you have made setLength and read synchronized but readByte and all the write methods are not synchronized. Probably I am having this doubt because of my not knowing the code properly but can you explain why you have done this? The cosmetic comments I had can be ignored if you want to. I consider them trivial. For whatever it is worth I wrote those comments in the form of a text file and am attaching the file to this issue.
        Hide
        V.Narayanan added a comment -

        Attached is the file containing the cosmetic issues I identified. Pls feel free to ignore them if you want to.

        Show
        V.Narayanan added a comment - Attached is the file containing the cosmetic issues I identified. Pls feel free to ignore them if you want to.
        Hide
        V.Narayanan added a comment -

        I am sorry I meant to say I was planning to paste the attachment internally only (but since the comments were trivial javadoc issues I decided against pasting it internally and pasted it as a external text file.)

        Show
        V.Narayanan added a comment - I am sorry I meant to say I was planning to paste the attachment internally only (but since the comments were trivial javadoc issues I decided against pasting it internally and pasted it as a external text file.)
        Hide
        Anurag Shekhar added a comment -

        Descriptions of path (derby-2379v2.diff)
        Fixed bugs found in Knuth and Narayanan reviews
        Removed my code to run tests in encrypted mode instead using the Decorator method for the same
        Enabled jdbcapi/BlobClob4BlobTest, jdbcapi/LobStreamsTest, jdbcapi/ClobUpdatableReaderTest, jdbc4/LobStreamTest to include testing in encrypted db.
        Fixed Javadoc warning and added throws clause in LOBFile methods
        Removed unused varraibles
        Removed redundant calls to file.length from read and write methods
        Added an exception in case setLength is called with a value longer than the file length.

        About synchronization
        I have decided to follow RandomAccessFile models (making this class thread unsafe). The user of this class has to ensure synchronization which is being taking care in this case by LOBStreamControl. added a note about it in java doc.

        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)

        there was bug in the test case modification i had done (as Naryanan pointed) so the bugs knut pointed in his review remained unexposed.All the tests I have enabled for encrypted mode test most of the new code.

        Show
        Anurag Shekhar added a comment - Descriptions of path (derby-2379v2.diff) Fixed bugs found in Knuth and Narayanan reviews Removed my code to run tests in encrypted mode instead using the Decorator method for the same Enabled jdbcapi/BlobClob4BlobTest, jdbcapi/LobStreamsTest, jdbcapi/ClobUpdatableReaderTest, jdbc4/LobStreamTest to include testing in encrypted db. Fixed Javadoc warning and added throws clause in LOBFile methods Removed unused varraibles Removed redundant calls to file.length from read and write methods Added an exception in case setLength is called with a value longer than the file length. About synchronization I have decided to follow RandomAccessFile models (making this class thread unsafe). The user of this class has to ensure synchronization which is being taking care in this case by LOBStreamControl. added a note about it in java doc. 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) there was bug in the test case modification i had done (as Naryanan pointed) so the bugs knut pointed in his review remained unexposed.All the tests I have enabled for encrypted mode test most of the new code.
        Hide
        Knut Anders Hatlen added a comment -

        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);
        Show
        Knut Anders Hatlen added a comment - 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);
        Hide
        Anurag Shekhar added a comment -

        derby-2379v2-conflict.diff:
        Resolved conflicts of derby-2379v2, no real change from derby-2379v2.

        Show
        Anurag Shekhar added a comment - derby-2379v2-conflict.diff: Resolved conflicts of derby-2379v2, no real change from derby-2379v2.
        Hide
        Anurag Shekhar added a comment -

        I am still working on the comments Knuth has posted but mean while i uploaded a new patch to resolve merge conflicts, in case some one wants to look at the patch.

        Show
        Anurag Shekhar added a comment - I am still working on the comments Knuth has posted but mean while i uploaded a new patch to resolve merge conflicts, in case some one wants to look at the patch.
        Hide
        Anurag Shekhar added a comment -

        about derby-2379v3.diff

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

        Problem with extending RandomAccessFile is that I will need to intercept all the methods to either throw not supported exception or to include encryption.

        > * 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.

        i have this checking beacuase stream classes need to throw IndexOutOfBoundsException in case of negative length.

        > * 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().

        fixed

        > * 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.
        changed

        > * 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).

        changed

        > * 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.

        updated javadoc

        >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.

        added description of params and exceptions

        > 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.

        done

        > 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.

        fixed also added same code in seek method if new pos is larger than file size

        > 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".

        fixed

        > 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);

        changed

        Show
        Anurag Shekhar added a comment - about derby-2379v3.diff >The implementations of StorageRandomAccessFile normally extend >java.io.RandomAccessFile which implements all the methods of the >StorageRandomAccessFile interface. Could that help LOBFile as well? Problem with extending RandomAccessFile is that I will need to intercept all the methods to either throw not supported exception or to include encryption. > * 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. i have this checking beacuase stream classes need to throw IndexOutOfBoundsException in case of negative length. > * 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(). fixed > * 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. changed > * 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). changed > * 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. updated javadoc >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. added description of params and exceptions > 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. done > 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. fixed also added same code in seek method if new pos is larger than file size > 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". fixed > 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); changed
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for addressing my comments, Anurag! I will take a look at the latest patch and commit it if the tests pass and I don't find more issues.

        As to the discussion about LOBFile, even if we cannot let LOBFile implement the StorageFile interface, I still feel it would be cleaner to split the code for encrypted and non-encrypted LOBs. For instance, we could let LOBFile handle unencrypted LOBs, and have another class EncryptedLOBFile which extended LOBFile and handled the encrypted LOBs. But that could be addressed as a separate clean-up task later.

        Show
        Knut Anders Hatlen added a comment - Thanks for addressing my comments, Anurag! I will take a look at the latest patch and commit it if the tests pass and I don't find more issues. As to the discussion about LOBFile, even if we cannot let LOBFile implement the StorageFile interface, I still feel it would be cleaner to split the code for encrypted and non-encrypted LOBs. For instance, we could let LOBFile handle unencrypted LOBs, and have another class EncryptedLOBFile which extended LOBFile and handled the encrypted LOBs. But that could be addressed as a separate clean-up task later.
        Hide
        Knut Anders Hatlen added a comment -

        I have looked at the v3 patch, and it looks correct to me. However,
        I'm a bit worried about the performance impact this patch may have on
        unencrypted data.

        I would expect this to have close to zero overhead for unencrypted
        lobs, but I think the patch adds some of the overhead needed for the
        encrypted lobs to the unencrypted lobs as well. For instance, if we
        look a call to LOBStreamControl.read(long) on an unencrypted database,
        this is what happens today:

        • StorageRandomAccessFile.getFilePointer() is called to check
          whether we need to reposition with StorageRandomAccessFile.seek()
        • StorageRandomAccessFile.readByte() is called, and its return value
          is returned

        With the patch, this is what happens:

        • LOBFile.getFilePointer() is called to check whether we need to
          reposition. If we need to reposition, LOBFile.seek() is called,
          which triggers these operations:
        • one call to StorageRandomAccessFile.length() to check whether
          the new position is after the end of the file
        • one call to StorageRandomAccessFile.length() to check whether
          the new position is before the end of file, in which case
          StorageRandomAccessFile.seek() is called
        • update current position pointer in LOBFile
        • LOBFile.readByte() is called, which triggers these operations:
        • one call to StorageRandomAccessFile.length() to check whether
          we try to read after the end of the file
        • one call to StorageRandomAccessFile.getFilePointer() to see
          whether we need to reposition, in which case
          StorageRandomAccessFile.seek() is called
        • one call to StorageRandomAccessFile.readByte()
        • update current position pointer in LOBFile

        Similar overhead seems to have been added to all the read and write
        methods, and most of it is because we maintain the current position
        pointer for unencrypted lobs. I think most of the overhead could have
        been avoided if LOBFile had used an OO approach with a base class for
        unencrypted lobs and a sub-class for encrypted lobs.

        Show
        Knut Anders Hatlen added a comment - I have looked at the v3 patch, and it looks correct to me. However, I'm a bit worried about the performance impact this patch may have on unencrypted data. I would expect this to have close to zero overhead for unencrypted lobs, but I think the patch adds some of the overhead needed for the encrypted lobs to the unencrypted lobs as well. For instance, if we look a call to LOBStreamControl.read(long) on an unencrypted database, this is what happens today: StorageRandomAccessFile.getFilePointer() is called to check whether we need to reposition with StorageRandomAccessFile.seek() StorageRandomAccessFile.readByte() is called, and its return value is returned With the patch, this is what happens: LOBFile.getFilePointer() is called to check whether we need to reposition. If we need to reposition, LOBFile.seek() is called, which triggers these operations: one call to StorageRandomAccessFile.length() to check whether the new position is after the end of the file one call to StorageRandomAccessFile.length() to check whether the new position is before the end of file, in which case StorageRandomAccessFile.seek() is called update current position pointer in LOBFile LOBFile.readByte() is called, which triggers these operations: one call to StorageRandomAccessFile.length() to check whether we try to read after the end of the file one call to StorageRandomAccessFile.getFilePointer() to see whether we need to reposition, in which case StorageRandomAccessFile.seek() is called one call to StorageRandomAccessFile.readByte() update current position pointer in LOBFile Similar overhead seems to have been added to all the read and write methods, and most of it is because we maintain the current position pointer for unencrypted lobs. I think most of the overhead could have been avoided if LOBFile had used an OO approach with a base class for unencrypted lobs and a sub-class for encrypted lobs.
        Hide
        Anurag Shekhar added a comment -

        Thanks for the earlier reviews and comments.

        I have separated EncryptedLOBFile to handle encrypted file. Now the LOBFile acts like a wrapper for StorageRandomAccessFile forwarding all calls to StorageRandomAccessFile.

        I have noticed some failures because I haven't set path for old version jars. I will run the tests again and will post the results.

        Show
        Anurag Shekhar added a comment - Thanks for the earlier reviews and comments. I have separated EncryptedLOBFile to handle encrypted file. Now the LOBFile acts like a wrapper for StorageRandomAccessFile forwarding all calls to StorageRandomAccessFile. I have noticed some failures because I haven't set path for old version jars. I will run the tests again and will post the results.
        Hide
        Anurag Shekhar added a comment -

        Thanks for the earlier reviews and comments.

        I have introduced EncryptedLOBFile and separated code for unencrypted lob in this. Now the LOBFile is just a wrapper over StorageRandomAccessFile and forwarding all calls to it.

        While running the test I saw some failure because I hadn't set old jar's path.

        I will run the tests again and will post the result after that.

        Show
        Anurag Shekhar added a comment - Thanks for the earlier reviews and comments. I have introduced EncryptedLOBFile and separated code for unencrypted lob in this. Now the LOBFile is just a wrapper over StorageRandomAccessFile and forwarding all calls to it. While running the test I saw some failure because I hadn't set old jar's path. I will run the tests again and will post the result after that.
        Hide
        Anurag Shekhar added a comment -

        i ran the tests again every thing passes except for java.security.AccessControlException in Encryption suite.

        Show
        Anurag Shekhar added a comment - i ran the tests again every thing passes except for java.security.AccessControlException in Encryption suite.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks Anurag! Committed revision 541981.

        Show
        Knut Anders Hatlen added a comment - Thanks Anurag! Committed revision 541981.
        Hide
        Knut Anders Hatlen added a comment -

        Would it be possible to rewrite seek(), setLength() and readByte() in EncryptedLOBFile so that they only called RandomAccessFile.length() once?

        Show
        Knut Anders Hatlen added a comment - Would it be possible to rewrite seek(), setLength() and readByte() in EncryptedLOBFile so that they only called RandomAccessFile.length() once?
        Hide
        Knut Anders Hatlen added a comment -

        It seems like the patch caused failures in the regression tests on some platforms (see DERBY-2715). Anurag, could you take a look at it?

        Show
        Knut Anders Hatlen added a comment - It seems like the patch caused failures in the regression tests on some platforms (see DERBY-2715 ). Anurag, could you take a look at it?
        Hide
        Knut Anders Hatlen added a comment -

        Could this check-in have caused DERBY-2718?

        Show
        Knut Anders Hatlen added a comment - Could this check-in have caused DERBY-2718 ?
        Hide
        Anurag Shekhar added a comment -

        seek(), setLength() and readByte() in EncryptedLOBFile to avoid multiple calls to RandomAccessFile.length () method

        Show
        Anurag Shekhar added a comment - seek(), setLength() and readByte() in EncryptedLOBFile to avoid multiple calls to RandomAccessFile.length () method
        Hide
        Knut Anders Hatlen added a comment -

        Thanks Anurag. Committed revision 543483.

        Show
        Knut Anders Hatlen added a comment - Thanks Anurag. Committed revision 543483.
        Hide
        Myrna van Lunteren added a comment -

        Is this issue complete? If so, it can be closed. Does it need a release note - i.e. is there existing application impact?

        Show
        Myrna van Lunteren added a comment - Is this issue complete? If so, it can be closed. Does it need a release note - i.e. is there existing application impact?
        Hide
        Kristian Waagan added a comment -

        Based on the lack of feedback I'm assuming all work has been completed for this issue. It is already resolved as fixed so I'm closing it.

        Show
        Kristian Waagan added a comment - Based on the lack of feedback I'm assuming all work has been completed for this issue. It is already resolved as fixed so I'm closing it.

          People

          • Assignee:
            Anurag Shekhar
            Reporter:
            Anurag Shekhar
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development