Blob.getBinaryStream(long,long) is off by one for the pos+len check

• Key: DERBY-4060
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Kristian Waagan
• Reporter: Trejkaz
• Component/s: JDBC, Network Client
• Affects Version/s: 10.3.3.1, 10.4.2.0, 10.5.1.1
• Fix Version/s: 10.3.3.1, 10.4.2.1, 10.5.1.1

Description

If you have a BLOB of length 20, and call blob.getBinaryStream(11,10), it will give you an error:

    java.sql.SQLException: Sum of position('11') and length('10') is greater than the size of the LOB.

This is following word for word an error in the JDBC Javadoc:

        SQLException - if pos is less than 1 or if pos is greater than the number of bytes in the Blob or if pos + length is greater than the number of bytes in the Blob

So it's checking 11 + 10 > 20, but it should check 11 + 10 > 21 (pos + len > blob.length() + 1) to allow reading the last byte.

The Javadoc for Clob.getCharacterStream(long,long) has similar wording so it may have the same issue.

Likewise, the client driver may have the same issue -- I haven't yet checked.

Comments

[Kristian Waagan]

This issue is present for both Blob and Clob, and also in the client driver.

[Kristian Waagan]

The patch 'derby-4060-1a-sub_stream_fix.diff' fixes the reported bug.
The changes in the production code consist of subtracting one from the position when checking against the length.
Using the example from the report;
 o current: 11 + 10 > 20 = true (exception thrown)
 o with patch: (11 -1) + 10 > 20 = false (check passes)

I made some more changes to the test code. Three tests were added, one for Blob and two for Clob (one Clob loaded with latin chars only, one loaded with CJK chars).
In addition, CharAlphabet-objects cannot be shared between readers. I dealt with this by adding getClone method to the CharAlphabet-class, and getting a clone when creating the LoopingAlphabetReader. This should maybe be changed in a more robust way.
It should also be noted that the method draining the stream and returning the last byte/char, performs more checks than usual.

Patch ready for review.

[Kristian Waagan]

Attached an updated patch, adding two more comments.

[Kristian Waagan]

Committed patch 1b to trunk with revision 749235.

[Kristian Waagan]

The fix seems to merge cleanly with both 10.4 and 10.3 (I haven't run tests yet).
I'll wait for a few days to see if any comments or problems show up, and if not I'll backport the fix.

[Knut Anders Hatlen]

Should we also update the javadoc comments in EmbedBlob and EmbedClob (they still say that we throw SQLException if pos+length is greater than the number of bytes/characters)?

Same goes for the error message (XJ087.S) which says "Sum of position('{0}') and length('{1}') is greater than the size of the LOB."

[Kristian Waagan]

Thank you for the comment, Knut Anders.
Patch 2a changes the JavaDoc and the error message.
Feel free to comment on the wording.

I plan to commit this on Monday.
Patch ready for review.

[Knut Anders Hatlen]

The patch looks good to me. (I was wondering what we should do with the translations, but it appears that we are only required to do something if the message parameters are changed, see comment near the top of messages.xml. For now it's probably better to leave the (possibly inaccurate) translated messages than to remove them.)

[Kristian Waagan]

Backported patch 1b to 10.4 with revision 750409 and to 10.3 with revision 750410.

[Kristian Waagan]

Committed patch 2a to trunk with revision 751637.
Backported to 10.4 and 10.3 with revisions 751639 and 751640.

This concludes my work on this issue.