JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-6790

Possible long integer overflow in fixnumStep in RubyNumeric.java

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Incomplete
    • Affects Version/s: JRuby 1.6.7
    • Fix Version/s: None
    • Component/s: Core Classes/Modules
    • Labels:
      None
    • Environment:
      Microsoft Windows, but if I'm correct the problem is in the Java source code so wont' be environment dependent, and is probably present in most (all) JRuby versions?
    • Number of attachments :
      0

      Description

      There seem to be long integer overflow problems in Fixnum#step illustrated by the (J)Ruby code just below. I've had a go at writing a patch (see below) and tried to put that as a "pull request" combined with my suggested patches for JRUBY-6777.

      nn = 256
      nn *= 256
      nn *= nn * nn * (-nn / 2)
      minv = nn
      maxv = -(nn + 1)
      p minv, maxv

      def test_step( fromv, tov, stepv )
      puts "#r #

      {fromv}

      .step( #

      {tov}

      , #

      {stepv}

      )"
      kt = 0
      v = fromv.step(tov, stepv) do |n|
      kt += 1
      puts "#r n= #

      {n}

      kt=#

      {kt}

      "
      break if kt >= 9
      end
      puts "#r v= #

      {v.inspect}

      "
      v
      end

      puts ; puts "this works properly"
      test_step( maxv - 3, maxv - 1, 1 )

      puts ; puts "this overflows"
      test_step( maxv - 3, maxv - 0, 1 )

      puts ; puts "this also overflows"
      test_step( maxv - 7, maxv - 1, 5 )

      This is a suggested patch:

      private static void fixnumStep(ThreadContext context, Ruby runtime, long value, long end, long diff, Block block) {
      if (diff == 0) throw runtime.newArgumentError("step cannot be 0");
      // tempID: Patch by colinb2r on Github to deal with possible integer overflows;
      // Now we must check for possible integer overflow in "i += diff".
      // We can do that by checking for integer overflow in every "i += diff".
      // We can do that by adapting code from RubyFixnum.java (or elsewhere),
      // or by using "long iPrevious", and (for example) for "diff > 0"
      // checking that if "iPrevious >= 0" then new value of i is also > 0.
      // (Or possibly we could use a boolean iPreviousGTzero.)
      // But we don't want to slow down the "for" statement.
      // So instead run the "for" statement to an adjusted end value "endvv"
      // which is sufficiently far from Long.MAX_VALUE or Long.MIN_VALUE
      // that the last "i += diff" in the "for" statement won't overflow,
      // and after the "for" statement check if we should yield another value,
      // which will be ready and waiting in "i".
      long endvv, i;
      if (diff > 0) {
      endvv = Long.MAX_VALUE - diff;
      if (endvv > end) endvv = end;
      for (i = value; i <= endvv; i += diff)

      { block.yield(context, RubyFixnum.newFixnum(runtime, i)); }
      } else {
      endvv = Long.MIN_VALUE - diff;
      if (endvv < end) endvv = end;
      for (i = value; i >= endvv; i += diff) { block.yield(context, RubyFixnum.newFixnum(runtime, i)); }

      }
      if (endvv != end)

      { block.yield(context, RubyFixnum.newFixnum(runtime, i)); }

      }

        Activity

        Hide
        Colin Bartlett added a comment -

        It's embarrassing to have to suggest a patch to one's own suggested patch, but here goes:

        Replace:
        if (endvv > end) endvv = end;
        By:
        // the "|| end < value" deals with: endvv < end < value;
        // if we exclude it then "if (endvv != end)" yields "i == value";
        if (endvv > end || end < value) endvv = end;

        Replace:
        if (endvv < end) endvv = end;
        By:
        // the "|| end > value" deals with: endvv > end > value;
        // if we exclude it then "if (endvv != end)" yields "i == value";
        if (endvv < end || end > value) endvv = end;

        Show
        Colin Bartlett added a comment - It's embarrassing to have to suggest a patch to one's own suggested patch, but here goes: Replace: if (endvv > end) endvv = end; By: // the "|| end < value" deals with: endvv < end < value; // if we exclude it then "if (endvv != end)" yields "i == value"; if (endvv > end || end < value) endvv = end; Replace: if (endvv < end) endvv = end; By: // the "|| end > value" deals with: endvv > end > value; // if we exclude it then "if (endvv != end)" yields "i == value"; if (endvv < end || end > value) endvv = end;
        Hide
        Charles Oliver Nutter added a comment -

        In the future, it would help to provide a link to your pull request in your bug report, and a link to the bug report in the pull request

        I'll have a look at this today. Thanks for your help!

        Show
        Charles Oliver Nutter added a comment - In the future, it would help to provide a link to your pull request in your bug report, and a link to the bug report in the pull request I'll have a look at this today. Thanks for your help!
        Hide
        Charles Oliver Nutter added a comment -

        https://github.com/jruby/jruby/pull/236

        I added some comments there about the patch.

        Will mark for 1.7pre2.

        Show
        Charles Oliver Nutter added a comment - https://github.com/jruby/jruby/pull/236 I added some comments there about the patch. Will mark for 1.7pre2.
        Hide
        Charles Oliver Nutter added a comment -

        We'll call this incomplete and track ongoing work in the other issues already filed.

        Show
        Charles Oliver Nutter added a comment - We'll call this incomplete and track ongoing work in the other issues already filed.
        Hide
        Charles Oliver Nutter added a comment -

        Additional fixes:

        commit ee963e52200617634eb11d9ffbf984b956f7fb21
        Author: Charles Oliver Nutter <headius@headius.com>
        Date:   Thu Nov 8 13:44:42 2012 -0600
        
            Fix JRUBY-6612, JRUBY-6777, JRUBY-6778, JRUBY-6779, JRUBY-6790
            
            [JRUBY-6612] some problems with JRuby seeming to not detect Java Long arithmetic overflows
            [JRUBY-6777] RubyFixnum.java - two methods fail to detect some long integer overflows
            [JRUBY-6778] Possible long integer overflow bug in Integer#succ in RubyInteger.java
            [JRUBY-6779] Strange behaviour of some Integer Ranges with Range#each - maybe an integer overflow problem?
            [JRUBY-6790] Possible long integer overflow in fixnumStep in RubyNumeric.java
            
            Patches by Colin Bartlett. Thank you!
        
        :100644 100644 93f83c8... 109b856... M	src/org/jruby/RubyInteger.java
        :100644 100644 b631a40... 7ecf098... M	src/org/jruby/RubyNumeric.java
        :100644 100644 3ed0b55... d15c2ed... M	src/org/jruby/RubyRange.java
        
        Show
        Charles Oliver Nutter added a comment - Additional fixes: commit ee963e52200617634eb11d9ffbf984b956f7fb21 Author: Charles Oliver Nutter <headius@headius.com> Date: Thu Nov 8 13:44:42 2012 -0600 Fix JRUBY-6612, JRUBY-6777, JRUBY-6778, JRUBY-6779, JRUBY-6790 [JRUBY-6612] some problems with JRuby seeming to not detect Java Long arithmetic overflows [JRUBY-6777] RubyFixnum.java - two methods fail to detect some long integer overflows [JRUBY-6778] Possible long integer overflow bug in Integer#succ in RubyInteger.java [JRUBY-6779] Strange behaviour of some Integer Ranges with Range#each - maybe an integer overflow problem? [JRUBY-6790] Possible long integer overflow in fixnumStep in RubyNumeric.java Patches by Colin Bartlett. Thank you! :100644 100644 93f83c8... 109b856... M src/org/jruby/RubyInteger.java :100644 100644 b631a40... 7ecf098... M src/org/jruby/RubyNumeric.java :100644 100644 3ed0b55... d15c2ed... M src/org/jruby/RubyRange.java

          People

          • Assignee:
            Charles Oliver Nutter
            Reporter:
            Colin Bartlett
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: