Derby
  1. Derby
  2. DERBY-5536

Client's ResultSet#getLong does not range check when converting from a DECIMAL column

    Details

    • Bug behavior facts:
      Deviation from standard, Embedded/Client difference, Wrong query result

      Description

      Derby's DECIMAL can contain an integer of 31 digits. This can overflow a long. The embedded JDBC driver catches this, but the client does not and can yield a corrupt long, cf enclosed repro.

      1. derby-5536.diff
        10 kB
        Dag H. Wanvik
      2. derby-5536.stat
        0.2 kB
        Dag H. Wanvik
      3. derby-5536-2.diff
        11 kB
        Dag H. Wanvik
      4. derby-5536-2.stat
        0.2 kB
        Dag H. Wanvik
      5. derby-5536-3.diff
        12 kB
        Dag H. Wanvik
      6. derby-5536-3.stat
        0.2 kB
        Dag H. Wanvik
      7. derby-5536-refactor.diff
        6 kB
        Dag H. Wanvik
      8. MissingRangeCheck.java
        4 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          The bug also is present for getInt, getShort and getByte sinc ethey rely on getLong. The error lies in am.Decimal#getLong, which contains this loop:

          // compute the integer part only.
          int leftOfDecimalPoint = length * 2 - 1 - scale;
          long integer = 0;
          if (leftOfDecimalPoint > 0) {
          int i = 0;
          for (; i < leftOfDecimalPoint / 2; i++)

          { integer = integer * 10 + signum * ((buffer[offset + i] & 0xF0) >>> 4); // high nybble. integer = integer * 10 + signum * (buffer[offset + i] & 0x0F); // low nybble. }

          :

          If the scale is high, say, 31, the accumulating value "integer" will wrap around here with no warning.

          Show
          Dag H. Wanvik added a comment - The bug also is present for getInt, getShort and getByte sinc ethey rely on getLong. The error lies in am.Decimal#getLong, which contains this loop: // compute the integer part only. int leftOfDecimalPoint = length * 2 - 1 - scale; long integer = 0; if (leftOfDecimalPoint > 0) { int i = 0; for (; i < leftOfDecimalPoint / 2; i++) { integer = integer * 10 + signum * ((buffer[offset + i] & 0xF0) >>> 4); // high nybble. integer = integer * 10 + signum * (buffer[offset + i] & 0x0F); // low nybble. } : If the scale is high, say, 31, the accumulating value "integer" will wrap around here with no warning.
          Hide
          Dag H. Wanvik added a comment -

          Uploading derby-5536 which changes the implementation of am.Decimal#getLong to allow it to detect overflow. If the number of digits indicate it can't happen we use an optimized path. Uncommented a overflow test case in ParameterMappingTest which was disabled due to this error and and added a new positive test case since we changed the implementation.

          Running regressions.

          Show
          Dag H. Wanvik added a comment - Uploading derby-5536 which changes the implementation of am.Decimal#getLong to allow it to detect overflow. If the number of digits indicate it can't happen we use an optimized path. Uncommented a overflow test case in ParameterMappingTest which was disabled due to this error and and added a new positive test case since we changed the implementation. Running regressions.
          Hide
          Dag H. Wanvik added a comment -

          Regressions passed.

          Show
          Dag H. Wanvik added a comment - Regressions passed.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a revised patch, derby-5536-2. Small test improvement only. Will commit soon, so please review.

          Show
          Dag H. Wanvik added a comment - Uploading a revised patch, derby-5536-2. Small test improvement only. Will commit soon, so please review.
          Hide
          Knut Anders Hatlen added a comment -

          The patch looks good to me.

          Might be worthwhile adding a comment saying that the conversion BigDecimal->BigInteger->BigDecimal in Decimal.getLong() is done to strip off the fraction part.

          Do the existing tests cover the case where a DECIMAL with non-zero scale and non-zero fraction part is converted to long? The new test cases only test numbers with no fraction part, I think.

          Show
          Knut Anders Hatlen added a comment - The patch looks good to me. Might be worthwhile adding a comment saying that the conversion BigDecimal->BigInteger->BigDecimal in Decimal.getLong() is done to strip off the fraction part. Do the existing tests cover the case where a DECIMAL with non-zero scale and non-zero fraction part is converted to long? The new test cases only test numbers with no fraction part, I think.
          Hide
          Dag H. Wanvik added a comment -

          Uploading patch #3 which address your comments, Knut. This tests a non-zero fractional part as well (rounding towards zero) and adds a comment as suggested.

          Show
          Dag H. Wanvik added a comment - Uploading patch #3 which address your comments, Knut. This tests a non-zero fractional part as well (rounding towards zero) and adds a comment as suggested.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dag. The changes look good to me. Maybe the select statement in the new test case for non-zero fraction should have an ORDER BY clause to ensure stable results, since the rows are not guaranteed to be returned in insertion order? And perhaps factor out that test case in a separate test method? (The test method in which it is currently located, is actually for testing that the SQLStates are correct.)

          Show
          Knut Anders Hatlen added a comment - Thanks, Dag. The changes look good to me. Maybe the select statement in the new test case for non-zero fraction should have an ORDER BY clause to ensure stable results, since the rows are not guaranteed to be returned in insertion order? And perhaps factor out that test case in a separate test method? (The test method in which it is currently located, is actually for testing that the SQLStates are correct.)
          Hide
          Dag H. Wanvik added a comment -

          Committed on trunk as svn 1229082, resolving.

          Show
          Dag H. Wanvik added a comment - Committed on trunk as svn 1229082, resolving.
          Hide
          Dag H. Wanvik added a comment -

          Thanks, Knut. I committed before I saw your latest comment. I'll address this in a follow-up patch.

          Show
          Dag H. Wanvik added a comment - Thanks, Knut. I committed before I saw your latest comment. I'll address this in a follow-up patch.
          Hide
          Dag H. Wanvik added a comment -

          Attaching a follow-up patch which factors out the test cases for DERBY-5536 into a separate fixture, adding "order by" to secure correct row retrieval order.

          Show
          Dag H. Wanvik added a comment - Attaching a follow-up patch which factors out the test cases for DERBY-5536 into a separate fixture, adding "order by" to secure correct row retrieval order.
          Hide
          Dag H. Wanvik added a comment -

          Committed the followup patch derby-5536-refactor as svn 1229481.

          Show
          Dag H. Wanvik added a comment - Committed the followup patch derby-5536-refactor as svn 1229481.
          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
          Hide
          Kathey Marsden added a comment -

          Assign to myself for backport

          Show
          Kathey Marsden added a comment - Assign to myself for backport
          Hide
          Kathey Marsden added a comment -

          Won't backport after all. This fix uses BigDecimal.longValueExact() which is not available in 1.4.2 so the fix does not build on 10.8.

          Show
          Kathey Marsden added a comment - Won't backport after all. This fix uses BigDecimal.longValueExact() which is not available in 1.4.2 so the fix does not build on 10.8.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development