Derby
  1. Derby
  2. DERBY-4519

Infinite loop in StreamFileContainer.writeColumn

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.0.2.1, 10.1.3.1, 10.2.2.0, 10.3.3.0, 10.4.2.0, 10.5.3.0, 10.6.1.0
    • Fix Version/s: 10.5.3.1, 10.6.1.0
    • Component/s: Store
    • Labels:
      None

      Description

      The offset and length argument have been swapped in the calls to InputStream.read(byte,offset, length) and write(byte,offset, length). This code is inside a do-while (true) loop, and the only normal way out is when InputStream.read returns -1. This will never happen since the stream is asked to read zero bytes. Derby ends up eating up all available CPU, limited to a single core / CPU.

      The bug hasn't been observed because Derby is materializing all values when calling this code. Enabling streaming capabilities in the sorter revealed it.

      1. derby-4519-2a-infinite_loop_fixes.diff
        1 kB
        Kristian Waagan
      2. derby-4519-1a-argument_swap.diff
        0.7 kB
        Kristian Waagan

        Activity

        Gavin made changes -
        Workflow jira [ 12488644 ] Default workflow, editable Closed status [ 12799616 ]
        Kristian Waagan made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Kristian Waagan added a comment -

        Closing the issue.

        Show
        Kristian Waagan added a comment - Closing the issue.
        Kristian Waagan made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Issue & fix info [Patch Available]
        Fix Version/s 10.5.3.1 [ 12314182 ]
        Resolution Fixed [ 1 ]
        Hide
        Kristian Waagan added a comment -

        Thanks for the review, Knut Anders.

        I committed patch 2a to trunk with revision 901165.
        Back-ported to the 10.5 branch with revision 901167 for good measure, but I don't expect that we will enable the affected code on that branch. For the same reason I'm not planning to back-port to older branches.

        Show
        Kristian Waagan added a comment - Thanks for the review, Knut Anders. I committed patch 2a to trunk with revision 901165. Back-ported to the 10.5 branch with revision 901167 for good measure, but I don't expect that we will enable the affected code on that branch. For the same reason I'm not planning to back-port to older branches.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Kristian. The patch looks good. +1

        Show
        Knut Anders Hatlen added a comment - Thanks, Kristian. The patch looks good. +1
        Kristian Waagan made changes -
        Attachment derby-4519-2a-infinite_loop_fixes.diff [ 12430870 ]
        Hide
        Kristian Waagan added a comment -

        Patch 2a deprecates patch 1a, and it also addresses the issue (and the nit) noted by Knut Anders.
        The buffer size will now be determined by InputStream.available() given that it returns a value between 64 and 8192 (8 KB).
        Since this code path doesn't appear to be reached currently, I don't expect any changes in behavior at this point.
        The fix is an incremental improvement regardless of the exact values of the limits , as the existing code would cause Derby to enter an infinite loop if executed.

        Patch ready for review.

        Show
        Kristian Waagan added a comment - Patch 2a deprecates patch 1a, and it also addresses the issue (and the nit) noted by Knut Anders. The buffer size will now be determined by InputStream.available() given that it returns a value between 64 and 8192 (8 KB). Since this code path doesn't appear to be reached currently, I don't expect any changes in behavior at this point. The fix is an incremental improvement regardless of the exact values of the limits , as the existing code would cause Derby to enter an infinite loop if executed. Patch ready for review.
        Hide
        Knut Anders Hatlen added a comment -

        Hi Kristian,

        The suggested bounding of the buffer size sounds fine. We should probably also add a short comment about why it's needed.

        Show
        Knut Anders Hatlen added a comment - Hi Kristian, The suggested bounding of the buffer size sounds fine. We should probably also add a short comment about why it's needed.
        Hide
        Kristian Waagan added a comment -

        Hi Knut,

        I think we can address the additional problem you spotted here as well. The question is which thresholds to use.
        Assuming available() will give reasonable answers in most cases (may very well be the case if we are reading from internal store streams), which limits should we impose on the buffer size?
        [64, 8192], which could look like something like this?
        (BufferedInputStream uses 8 KB as default buffer size)

        int bufferLen = Math.min(Math.max(in.available(), 64), 8192).

        Based on the experiences from DERBY-3791, I would be careful about defaulting to a largish size if we do get reasonable answers from available().

        Show
        Kristian Waagan added a comment - Hi Knut, I think we can address the additional problem you spotted here as well. The question is which thresholds to use. Assuming available() will give reasonable answers in most cases (may very well be the case if we are reading from internal store streams), which limits should we impose on the buffer size? [64, 8192] , which could look like something like this? (BufferedInputStream uses 8 KB as default buffer size) int bufferLen = Math.min(Math.max(in.available(), 64), 8192). Based on the experiences from DERBY-3791 , I would be careful about defaulting to a largish size if we do get reasonable answers from available().
        Hide
        Knut Anders Hatlen added a comment -

        Another problem with this code is that it uses InputStream.available() to decide the size of the read buffer. If available() returns 0 (which it is allowed to) an empty read buffer is used, and this will also lead to an infinite loop, I think.

        The patch looks like an improvement, though. Minor nit: Calling the read(byte[]) method that takes no offset/length arguments would be equivalent to read(bufData,0,bufferLen) and slightly simpler.

        Show
        Knut Anders Hatlen added a comment - Another problem with this code is that it uses InputStream.available() to decide the size of the read buffer. If available() returns 0 (which it is allowed to) an empty read buffer is used, and this will also lead to an infinite loop, I think. The patch looks like an improvement, though. Minor nit: Calling the read(byte[]) method that takes no offset/length arguments would be equivalent to read(bufData,0,bufferLen) and slightly simpler.
        Kristian Waagan made changes -
        Affects Version/s 10.6.0.0 [ 12313727 ]
        Issue & fix info [Patch Available]
        Kristian Waagan made changes -
        Field Original Value New Value
        Attachment derby-4519-1a-argument_swap.diff [ 12430747 ]
        Hide
        Kristian Waagan added a comment -

        Attached patch 1a, which swaps the length and offset arguments.

        Show
        Kristian Waagan added a comment - Attached patch 1a, which swaps the length and offset arguments.
        Kristian Waagan created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development