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

Possible long integer overflow bug in Integer#succ in RubyInteger.java

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JRuby 1.7.0.pre1
    • Fix Version/s: JRuby 1.7.0.pre2
    • Component/s: Core Classes/Modules
    • Labels:
      None
    • Environment:
      I'm running Microsoft Windows Vista with Java Client VM 1.6.0_14, but I think the problem is likely to independent of the platform.
    • Number of attachments :
      0

      Description

      This seems to be a problem in 1.7.0.preview1 and in 1.6.7; I haven't tried it in other JRuby versions, but I suspect it will be a problem in other JRuby versions.

      In JIRB or run as a JRuby program:

        v = 2**62 * -2  #=> -9_223_372_036_854_775_808
        v = -(v + 2)    #=>  9_223_372_036_854_775_806
        v = v.succ      #=>  9_223_372_036_854_775_807
        v = v.succ      #=> -9_223_372_036_854_775_808
        v = v.succ      #=> -9_223_372_036_854_775_807
      

      Looking at this code in RubyInteger.java it seems clear that the issue is "succ" does not check for Java long integer overflow for "getLongValue() + 1L".

      RubyInteger.java 1.7.0.preview1 line 243

          @JRubyMethod(name = {"succ", "next"})
          public IRubyObject succ(ThreadContext context) {
              if (this instanceof RubyFixnum) {
                  return RubyFixnum.newFixnum(context.getRuntime(), getLongValue() + 1L);
              } else {
                  return callMethod(context, "+", RubyFixnum.one(context.getRuntime()));
              }
          }
      

      Looking at: RubyFixnum.java 1.7.0.preview1 line 353

          public IRubyObject op_plus_one(ThreadContext context) {
              long result = value + 1;
              if (result == Long.MIN_VALUE) {
                  return addAsBignum(context, 1);
              }
              return newFixnum(context.getRuntime(), result);
          }
      

      suggests this possible patch:

          public IRubyObject succ(ThreadContext context) {
              if (this instanceof RubyFixnum) {
        // replace "return RubyFixnum.newFixnum(..." by something like:
                  return this.op_plus_one(context);  // will this work?
              } else {
                  return callMethod(context, "+", RubyFixnum.one(context.getRuntime()));
              }
          }
      

      or maybe this if there are performance (or other) reasons for using
      "return RubyFixnum.newFixnum(..."

          public IRubyObject succ(ThreadContext context) {
              if (this instanceof RubyFixnum) {
                  long fixnumvalue = getLongValue();
                  if (fixnumvalue != Long.MAX_VALUE) {
                      return RubyFixnum.newFixnum(context.getRuntime(),fixnumvalue + 1L);
                  }
                  return this.op_plus_one(context);  // will this work?
              } else {
                  return callMethod(context, "+", RubyFixnum.one(context.getRuntime()));
              }
          }
      

        Activity

        Hide
        Charles Oliver Nutter added a comment -

        Your fix looks quite appropriate. Will try it out.

        Show
        Charles Oliver Nutter added a comment - Your fix looks quite appropriate. Will try it out.
        Hide
        Charles Oliver Nutter added a comment -
        commit 4c70a11dff86b2dafc1fec8ac80cebf52801a06c
        Author: Charles Oliver Nutter <headius@headius.com>
        Date:   Fri Jul 20 21:52:20 2012 -0500
        
            Fix JRUBY-6778
            
            Possible long integer overflow bug in Integer#succ in RubyInteger.java
            
            Normal Java math allowed incrementing to overflow to negative
            numbers. Replaced with a call to the same Fixnum#+ logic so
            overflow triggers Bignum. Patch mostly by Colin Bartlett.
        
        :100644 100644 94dcbe8... 92731eb... M	src/org/jruby/RubyInteger.java
        
        Show
        Charles Oliver Nutter added a comment - commit 4c70a11dff86b2dafc1fec8ac80cebf52801a06c Author: Charles Oliver Nutter <headius@headius.com> Date: Fri Jul 20 21:52:20 2012 -0500 Fix JRUBY-6778 Possible long integer overflow bug in Integer#succ in RubyInteger.java Normal Java math allowed incrementing to overflow to negative numbers. Replaced with a call to the same Fixnum#+ logic so overflow triggers Bignum. Patch mostly by Colin Bartlett. :100644 100644 94dcbe8... 92731eb... M src/org/jruby/RubyInteger.java
        Hide
        Colin Bartlett added a comment -

        Just a thought that this in RubyFixnum.java:
        public IRubyObject op_plus_one(ThreadContext context) {
        long result = value + 1;
        if (result == Long.MIN_VALUE)

        { return addAsBignum(context, 1); }
        return newFixnum(context.getRuntime(), result);
        }
        could be re-written as this, which might be a bit clearer about what's happening?
        public IRubyObject op_plus_one(ThreadContext context) {
        // check for whether value + 1 will be long integer overflow
        if (value == Long.MAX_VALUE) { return addAsBignum(context, 1); }

        return newFixnum(context.getRuntime(), value + 1);
        }

        Show
        Colin Bartlett added a comment - Just a thought that this in RubyFixnum.java: public IRubyObject op_plus_one(ThreadContext context) { long result = value + 1; if (result == Long.MIN_VALUE) { return addAsBignum(context, 1); } return newFixnum(context.getRuntime(), result); } could be re-written as this, which might be a bit clearer about what's happening? public IRubyObject op_plus_one(ThreadContext context) { // check for whether value + 1 will be long integer overflow if (value == Long.MAX_VALUE) { return addAsBignum(context, 1); } return newFixnum(context.getRuntime(), value + 1); }
        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: