Derby
  1. Derby
  2. DERBY-3735

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • 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
    • Component/s: JDBC, Store
    • Labels:
      None

      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.

      1. derby-3735-2a.diff
        0.8 kB
        Kristian Waagan
      2. derby-3735-1b.diff
        12 kB
        Kristian Waagan
      3. derby-3735-1a.stat
        0.3 kB
        Kristian Waagan
      4. derby-3735-1a.diff
        12 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        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.

        Show
        Kristian Waagan added a comment - 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.
        Hide
        Knut Anders Hatlen added a comment -

        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.

        Show
        Knut Anders Hatlen added a comment - 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.
        Hide
        Kristian Waagan added a comment -

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

        Show
        Kristian Waagan added a comment - 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).
        Hide
        Kristian Waagan added a comment -

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

        Show
        Kristian Waagan added a comment - '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.
        Hide
        Kristian Waagan added a comment -

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

        Show
        Kristian Waagan added a comment - Committed 'derby-3735-2a.diff' to trunk with revision 672284.
        Hide
        Kristian Waagan added a comment -

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

        Show
        Kristian Waagan added a comment - Backported fix (patches 1b and 2a) to the 10.4 branch with revision 673030. Running tests on 10.3.
        Hide
        Kristian Waagan added a comment -

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

        This concludes the work on this issue.

        Show
        Kristian Waagan added a comment - Backported fix to the 10.3 branch with revision 673064. Tests ran without failures. This concludes the work on this issue.
        Hide
        Kristian Waagan added a comment -

        Closing.
        No problems reported.

        Show
        Kristian Waagan added a comment - Closing. No problems reported.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development