Details
-
Type:
Bug
-
Status:
Resolved
-
Priority:
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 :
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 #
.step( #
{tov}, #
{stepv} )"
kt = 0
v = fromv.step(tov, stepv) do |n|
kt += 1
puts "#r n= #
kt=#
{kt}"
break if kt >= 9
end
puts "#r v= #
"
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)
} 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)
}
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;