Incorrect position calculation in PositionedStoreStream with read(byte[],...)

• Key: DERBY-3735
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Kristian Waagan
• Reporter: Kristian Waagan
• Component/s: JDBC, Store
• Affects Version/s: 10.3.1.4, 10.4.1.3, 10.5.1.1
• Fix Version/s: 10.4.2.0, 10.5.1.1

Description

A bug in the methods 'read(byte[])' and 'read(byte[],int,int)' in PositionedStoreStream can cause the position variable to be set to an incorrect value.
The bug is only triggered if one of the two read methods is invoked after EOF of the underlying stream has been reached.

If the bug is triggered, the position will be decreased by one because the underlying stream returns -1. Subsequent reads will further decrease the position. This can cause EOF-exceptions (during repositioning) or incorrect data being returned in subsequent calls.

Comments

[Kristian Waagan]

Attaching 'derby-3735-1a.diff' that does the following:

 * PositionedStoreStream
    Fixed the position calculation.
    Implemented read(byte[]) by forwarding to read(byte[],int,int).

 * LoopingAlphabetStream
   Made it implement Resetable, so it can be used for testing PositionedStoreStream.

 * PositionedStoreStreamTest
   Some simple tests and a regression test for the bug.
   Takes less than half a second to run (no database access).

 * _Suite
    Added the new test to the store suite.


I ran the regression tests without failures.
Patch ready for review.

[Knut Anders Hatlen]

The patch looks correct to me, and the change looks well tested. +1 to commit.

A tiny nit: Now that read(byte[],int,int) has the same structure as read() which is located right below it, it would probably be better if they compared ret to the same number. Although it doesn't matter whether the condition is (ret > 0) or (ret > -1) in this case, the difference between the methods may be slightly confusing to those who read the code.

[Kristian Waagan]

Thanks for the review Knut Anders.

Committed 'derby-3735-1b.diff' to trunk with revision 671840.
Will backport to 10.4 and maybe 10.3.

Made a revision b of the patch due to a conflicting change in _Suite, and also addressed Knut Anders' nit (both methods now use the same condition).

[Kristian Waagan]

'derby-3735-2a.diff' fixes the bug introduced by the previous patch (1b)...

0 (zero) is of course a valid return value for "read()", and the position has to be incremented.
I changed the "read(byte[],int,int)" to check for -1 for consistency as well. This method will return 0 very seldom, in fact I think it requires a "user error" to happen; argument 'len' must be zero.

Regression tests run without failures.
Patch ready for review, but I will commit it shortly anyway to get rid of the tinderbox/nightly failures.

[Kristian Waagan]

Committed 'derby-3735-2a.diff' to trunk with revision 672284.

[Kristian Waagan]

Backported fix (patches 1b and 2a) to the 10.4 branch with revision 673030.
Running tests on 10.3.

[Kristian Waagan]

Backported fix to the 10.3 branch with revision 673064.
Tests ran without failures.

This concludes the work on this issue.

[Kristian Waagan]

Closing.
No problems reported.