Skip to content

Suggested patches to deal with integer overflow problems: search for #236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

colinb2r
Copy link

colinb2r in files

@headius
Copy link
Member

headius commented Jul 23, 2012

Ok, the fixes don't look bad in general, but I think you missed some logic that prevents things from compiling.

Could you please:

  • Fix any mistakes; I've noted one above.
  • Consider leaving the long math logic in multiplyFixnum. We try to keep all methods as small as possible to aid the JVM in optimizing them.
  • Run "ant test" and confirm everything builds and passes tests.

Thanks again!

@headius
Copy link
Member

headius commented Jul 23, 2012

@colinb2r
Copy link
Author

On Mon, Jul 23, 2012 at 3:00 PM, Charles Oliver Nutter <
reply@reply.github.com

wrote:

Ok, the fixes don't look bad in general, but I think you missed some logic
that prevents things from compiling.

Could you please:

  • Fix any mistakes; I've noted one above.
  • Consider leaving the long math logic in multiplyFixnum. We try to keep
    all methods as small as possible to aid the JVM in optimizing them.
  • Run "ant test" and confirm everything builds and passes tests.

Thanks again!

Sorry for my delay in replying.

I've had a problem using ant.

I think I've downloaded and installed ant correctly and set appropriate
Microsoft Windows/DOS paths and environment variables, but I get an error
trying to build JRuby from the source files.

That's happened on two different PCs and trying to compile JRuby source for
1.7.0.preview1 and for 1.6.6.

I'm going to try to narrow down where the error occurs and then if
necessary (and it probably will be necessary) try to get help from an
appropriate JRuby forum.

If I can't do all that within two to three days, then I'll resort to my
Plan C.

I've created a JRuby stand-alone program which attempts to test for the
integer overflow problems I think I've found so far.

I've just tried to upload it to gist.github.com, but that seems to be
taking forever, (as is Pastie.org) so I'll attach it here.

@headius
Copy link
Member

headius commented Aug 1, 2012

I did not see you attach anything...did I miss something?

Still very much interested in your patches and test cases. We're looking to push out JRuby 1.7pre2 end of this week, so now's the time!

@colinb2r
Copy link
Author

colinb2r commented Aug 2, 2012

On Wed, Aug 1, 2012 at 9:33 PM, Charles Oliver Nutter <
reply@reply.github.com

wrote:

I did not see you attach anything...did I miss something?

Still very much interested in your patches and test cases. We're looking
to push out JRuby 1.7pre2 end of this week, so now's the time!


Reply to this email directly or view it on GitHub:
#236 (comment)

The good news is that at about 2012-08-02_t_10-00_UTC today I seem to have
managed to successfully compile JRuby source for 1.6.7.2, so I'm about to
test my patches for the overflows from a compiled JRuby using the test
cases I generated.

I'll have another go at putting the JRuby file for the test cases on GitHub
and/or Pastie.

I'll send you an update at about 14:00_UTC today.

@colinb2r
Copy link
Author

colinb2r commented Aug 2, 2012

On Wed, Aug 1, 2012 at 9:33 PM, Charles Oliver Nutter <
reply@reply.github.com

wrote:

I did not see you attach anything...did I miss something?

Still very much interested in your patches and test cases. We're looking
to push out JRuby 1.7pre2 end of this week, so now's the time!


Reply to this email directly or view it on GitHub:
#236 (comment)

I've compiled jruby-1.7.0.preview1 and a version of jruby-1.7.0.preview1
with my suggested patches (slightly changed form those posted).

Because of my unfamiliarity with GitHub, not to mention the JRuby test
suite, for speed (I hope yours as well as mine - I've tried to make the
changes as easy to find and amend - if you want to - as possible) I'm
attaching a zipfile.

If you don't find the zipfile attached and/or can't access what's in it
and/or would like the files sent in a different way please feel free to
email me directly at colinb2@gmail.com. I did try to find a simple way to
put the zipfile on the web, but I didn't find an obvious way quickly, hence
this email first, then back to trying that.

So attached is (I hope) a zipfile with:

  • Three original jruby-1.7.0.preview1 java files, renamed to *java.txt, so
    people can check I'm starting from the latest version,
    or at least close to it;
  • Three patched jruby-1.7.0.preview1 java files: a quick way to find where
    the patches are in the files is to search in each file for "/-", or to be
    precise "//-", with the patches indicated by a "//+";
    Basically what I've done is to comment out the old code - sometimes just
    one line, sometimes a whole method - and out my suggested new code just
    after the commented out code.
  • a JRuby file with the overlong name: jruby-test-long-overflow-patches.rb
    which sets up some intentionally awkward test cases, at the moment 39 of
    them.
    MRI passes all 39 (which I'd expect, because MRI overflow problems, if
    any, are for different integer values);
    jruby-1.6.7.2 passes 28, fails 11;
    jruby-1.7.0.preview1 passes 28, fails 11, 10 of which are the same fails
    as jruby-1.6.7.2;
    jruby-1.7.0.preview1-patched-2012-08-02_t_16-09_UTC passes 36 and fails 3:
    the 3 fails are for: RubyRange.java#each, RubyRange.java#fixnumStep

Changes are to the following files: I've given reasons for the changes in
comments, which I hope can be followed. I'll address some of your point in
emails to me in a follow-up email.

  • RubyFixnum.java: op_plus_two, op_minus_two, op_mul, idivLong,
    divmodFixnum;
    this is my first mention of problems with op_plus_two and op_minus_two,
    but if you look at them I think it's fairly obvious that there are
    potential long integer overflow problems, and they do seem to be used in
    org.jruby.runtime.invokedynamic.MathLinker.java
  • RubyInteger.java: succ;
  • RubyNumeric.java: fixnumStep

@colinb2r
Copy link
Author

colinb2r commented Aug 2, 2012

Headius: "Is there a reason the fixes above couldn't just go in multipleFixnum (this one or the one that takes long)?

Reply:
I assume you mean multiplyFixnum?
Possibly not - for now I was trying to limit my changes to the code, in particular preserving the basic structure.
Since you mention it, then subject to optimization issues (and I leave those to you and others who know about the JRE), and subject to methods not actually being used elsewhere (after sending this email I'll try searching the JRuby Java code for any uses of the methods), then looking at the code:

  1. I don't understand why multiplyFixnum is used as an intermediary between the two "op_mul"s.
  2. Could the "multiplyOther" code just go into the first "op_mul" ?
  3. If the second "op_mul" is only used by the first "op_mul" via the private "multiplyFixnum" perhaps all the code in the second "op_mul" (original or patched) could go in the first "op_mul" ?

@colinb2r colinb2r closed this Aug 2, 2012
@colinb2r colinb2r reopened this Aug 2, 2012
headius added a commit that referenced this pull request Aug 2, 2012
Fixes issues in RubyInteger and RubyFixnum wrt Java long overflow.

Issues remain in RubyNumeric, based on Colin's test script, but he
instructed me to ignore those patches for now.
@headius
Copy link
Member

headius commented Aug 2, 2012

Ok! I have applied your changes to RubyInteger and RubyFixnum, which brings the failure count in your test script (https://gist.github.com/3240405) to 7 NotOK, all in RubyNumeric. Nearly there!

@headius
Copy link
Member

headius commented Aug 2, 2012

@colinb2r Suggestions for future patches:

  • Make sure you work against current JRuby source. There were some older bits in your recent patches that I had to manually back-patch.
  • If you are checking out JRuby with git, git diff will produce a nice patch output you can paste somewhere (like https://gist.github.com).
  • Don't reformat unrelated code; I had to drop several diffs that were simply reformatting.

Thanks again for the patches! Looking forward to seeing RubyNumeric finished up!

@colinb2r
Copy link
Author

colinb2r commented Aug 3, 2012

On Thu, Aug 2, 2012 at 9:38 PM, Charles Oliver Nutter <
reply@reply.github.com

wrote:

@colinb2r Suggestions for future patches:

  • Make sure you work against current JRuby source. There were some
    older bits in your recent patches that I had to manually back-patch.

Sorry about that: when I was having a go at pushing and pulling I think I
managed to get the "up-to-date" 1.7.0.preview1, but yesterday I was working
with the JRuby download page version.

  • If you are checking out JRuby with git, git diff will produce a nice
    patch output you can paste somewhere (like https://gist.github.com).
  • Don't reformat unrelated code; I had to drop several diffs that were
    simply reformatting.

Sorry.

Thanks again for the patches! Looking forward to seeing RubyNumeric
finished up!


Reply to this email directly or view it on GitHub:
#236 (comment)

"Ok! I have applied your changes to RubyInteger and RubyFixnum, which
brings the failure count in your test script (
https://gist.github.com/3240405) to 7 NotOK, all in RubyNumeric. Nearly
there!"

Well, "Nearly there"? No and Yes.

The "No" is that I seem to be quite good at generating test cases for
Integer looping constructs which MRI passes but JRuby sometimes fails,
including my patches.

The Yes is that I think I understand why my RubyNumeric.java fixnumStep
patch sometimes fails, and that there is an easy fix which also makes the
code slightly simpler. I'm implementing and testing that now.

I've put a revised jruby-test-long-overflow-patches.rb here
https://skydrive.live.com/?cid=01F6D7100ADB0423&id=1F6D7100ADB0423%21105&sc=documents

The following is an extract from the revised
"jruby-test-long-overflow-patches.rb" which has a command line option to
restrict the test cases to the really awkward ones for JRuby, for example:
jruby jruby-test-long-overflow-patches.rb -xxx
will only run the really awkward cases.

*** extract from the revised "jruby-test-long-overflow-patches.rb":

State of play at 2012-08-03_Fri_t_15-55_UTC:

  • patches which don't involve #each, #step, #upto, #downto seem ok;
  • patch for RubyRange.java fixnumEach seems OK,
    but that was more by luck than judgment:
    it's probably better to use code similar to correct code for
    RubyInteger.java fixnumUpto, fixnumDownto;
    RubyNumeric.java fixnumStep;
    RubyRange.java fixnumStep;
  • current patches (if any) for:
    RubyInteger.java fixnumUpto, fixnumDownto;
    RubyNumeric.java fixnumStep;
    RubyRange.java fixnumStep;
    fail for an unusual set or arguments, but they do fail.
    BUT: I think I have found a slight simplification of my patch
    for RubyNumeric.java fixnumStep which gives a general way
    of dealing with these looping constructs,
    and I'm now implementing and testing it.
    SUMMARY: for now I'd suggest not using any of my existing patches
    for the looping methods with integer overflow problems,
    on the grounds that one might as well temporarily stick
    with existing fails rather than have replacements with different fails.
    (There is Samuel Beckett's: "Fail; try again; fail better",
    and I think - but am not sure - that my looping patches did
    "fail better" in the sense that they were likely to fail less often
    than the current code.
    But for now I'm concentrating on testing what I think will always work,
    rather than spend time working out which "failed better".)

@headius
Copy link
Member

headius commented Aug 3, 2012

When everything is wrapped up we'll pull in your test script as a regression test (or contribute it to MRI), but feel free to keep updating it.

@headius
Copy link
Member

headius commented Sep 29, 2012

I'm going to close this...there's more work to do but we should do it in smaller doses.

@headius headius closed this Sep 29, 2012
eregon added a commit that referenced this pull request Apr 27, 2016
32e22d6 Ruby 2.4 raises a TypeError for invalid step args
8466e7c Fixed issues in Kernel#rand specs
3e0334a Removed using BigDecimal in rand specs.
9b8927d String#dup returns copy when no modification made.
a722922 Adds Numeric#step keyword arguments specs.
5114a66 Fixed edge case where small BigDecimal ranges were erroring
ad9be02 Fix float ranges where diff of 0
c0e852a Fixed rand() for degenerative empty range
bf42261 Only run frozen string literals specs on Ruby >= 2.3
faea24f Add two specs for new arrays with simple Fixnums
bf9ad3a Ensure String#byteslice normalizes indices by byte length, not character length.
b892ade BasicObject#instance_eval evaluates string with given filename and linenumber
946967e yield uses captured block of a block used in define_method
3e09f4e Spec that frozen string literals don't change what defined? returns, to protect against implementations that desugar the call away.
3f436dc Where we're testing the freeze command line flag, we shouldn't call .freeze.
47690be Where we're testing the freeze magic comment, we shouldn't call .freeze.
3d062e3 Add a spec for string literal freeze magic comment, where there is the same literal but in a file without the comment.
b9c349e Add command-line specs for --enable-frozen-string-literal.
b6c95d1 Add a spec for string literal freeze magic comment, where the literals are in different files but have different encodings.
db22ba5 Add a spec for string literal freeze magic comment, where the literals are in different files.
f8bfdb1 Add a spec for string literal freeze magic comment with two literals but the same content.
8a76900 Add a spec for string literal freeze magic comment with one literal.
b70af78 Add a spec for the special String#freeze syntax.
b3866cb New spec testing that compare_by_identity Hash do not call #hash.
0ade4ee Wait for the main thread to be interrupted in Thread#abort_on_exception spec
30cdf15 Remove bad rb_thread_call_without_gvl2 spec.
75c8c21 Merge pull request #248 from odaira/myContribution
99d20dc In AIX, when a pipe is readable, select(2) returns even the write side of the pipe as "readable"
b60523a Merge pull request #247 from nobu/overridable-OBJDIR
c5a98ec Make OBJDIR overridable
68dd505 Show our build status on Windows
d7a5a7b File.new/open changes permissions of existing files on MRI on Windows
2077fec Try to write a more sensible spec for IO#write on Windows
5dae610 Try to read in text mode for IO#write spec on Windows
55cb889 Use local variables in Module#ancestors
2b3bf9e Merge pull request #245 from wied03/ancestors
8ad6780 Better test Module#ancestors cases
e5765b1 Merge pull request #246 from wied03/class_to_s
723e4c9 Merge pull request #244 from wied03/master
7bf3a56 Test singleton class cases with `Class#to_s`
2341203 Test a strange module case compare edge case
4b6dd97 Merge pull request #243 from wied03/master
9c14562 Fill spec change from e60792ca2fe08ff2bb2f00eff7c6d70547cfc42e crashes Opal, so don't execute on that platform
ac7f701 Merge pull request #241 from wied03/master
a274931 First cut of Pathname#relative_path_from
4879393 Merge pull request #242 from znz/fix-typo
1a34bb3 Fix a typo
bc2614f Cleanup and reorganize a bit extensions spec_helper
231f0ec Merge pull request #240 from nobu/no-capi-signatures
1829feb Fix .gitignore file to only consider files at the root
31252bd Move extension files to ext/RUBY_NAME/RUBY_VERSION
4834b1a No CAPI signature files
663c7a0 Add sentinel
71d6bde Remove mri.h as MRI should have all C-API functions spec'd in ruby/spec
d294c56 Remove rb_inspecting_p and rb_protect_inspect specs since it is not standard
e60792c Remove deviates_on and use numeric helpers for Array#fill
b85c5f2 Remove extended_on and give proper describe blocks to rb_thread_blocking_region and such
0a9af62 Remove rb_str_ptr and rb_str_ptr_readonly since they are Rubinius-specific
d6c3234 Remove extended_on for missing functionality
28c64c8 rb_obj_frozen_p exists on other implementations as well
1c00935 Merge pull request #239 from odaira/myContribution2
e2065d8 Merge pull request #238 from odaira/myContribution
7805965 AIX does not allow mkdir(2) to set a sticky bit
c45706f Fix typo: 1755 intended to be 01755
ebfaf9d Exempt AIX in core/file/lchmod_spec.rb because it does not support lchmod(2)
260cdc3 Try a third way to pass the system spec and no issue console warnings
7462137 adapt expectation for exit status after SIGKILL on Windows
0bb8a49 Merge pull request #237 from iliabylich/add-tests-for-lbrace-arg-cases
a3dc36f quarantine problematic name resolving spec
ec8e369 SO_REUSEADDR seems to have a slightly different meaning in Windows
7784171 A getaddrinfo spec is also unsupported on Windows
bfc9162 resolv with localhost seems not working on Windows
6d7ea9a Try to fix a system spec on Windows
8c87aba IO.copy_stream with IO with offsets is not supported on Windows
6c66468 File::Stat#nlink seems to be unreliable on Windows
b73e686 Add tests for method calls that have a space between method name and parentheses.
82f728c Merge pull request #236 from iliabylich/add-specs-for-arity/parameters-for-attr_reader/writer
c54cc3a Added specs for Method#arity and Method#parameters for methods generated by attr_reader/attr_writer.
9b45a0d Merge pull request #235 from wied03/master
0a570d9 Add Pathname#join specs, enhance ::new
d6a94f7 Shell variable expansion in Kernel#system is spec on Windows.
64dba27 Remove libgmp from .travis.yml as ruby-head should not need it anymore.
fa55c43 Merge pull request #234 from odaira/myContribution
8b77012 Default error message returned by stderror(3) is "Error <errno> occurred" on AIX
ec4c0d0 Allow both US_ASCII and ASCII_8BIT if fs encoding is unset (LANG=C)
d9dd997 Simplify File::Stat#inspect and do not expect birthtime in the output on Windows.
11544a8 Do not try to change the {a,m}time of an opened file in File::Stat#<=> specs
b3ff895 Try to create files first in File::Stat#<=>.
37df92f Try to simplify the File#new spec with permissions.
a8363e9 chown is no-op on Windows
bd23a74 Math.lgamma(-0.0) is only guaranteed on 2.4 for now.
e2e203b Merge pull request #233 from nobu/feature/Math#lgamma-0.0
a1e623f Math.lgamma returns [infinity, -1] on -0.0
d5e21c8 Move the #include triggering #included outside fixtures.
0e11702 Merge pull request #232 from wied03/super_again
0683a53 Fix another super case
bbe5d48 Merge pull request #231 from wied03/more_super
a081c74 Test more super edge cases
c18e55b Merge pull request #229 from wied03/super_stuff
b04d2fe Merge pull request #228 from nobu/trailing-spaces
bd89b79 Merge pull request #227 from wied03/master
cd06cdd super enhancement
9059448 remove trailing spaces
a87ca8a Cover cases where the backslash itself is escaped
932fa26 Update Coverage specs to not filter the result if Ruby >= 2.4.
45935f3 Merge pull request #225 from nobu/bug/Dir.pwd-C
6d5daba Merge pull request #226 from ruby/try_with_gmp
7f04132 Fix Dir.pwd encoding
f3a55e1 libgmp-devel apt package
f90428f UNIX sockets only on UNIX
dfdb6d0 Make clear that not.existing is not an extension
b0bedcb Avoid warning by redirecting stderr on Windows
2ceb3bd Avoid changing LC_ALL, which has also no effect on Windows
26086ad Math.frexp returns an unspecified value for exp if value is NaN.
6cf4f32 File.expand_path does not care about the external Encoding on Windows
49701ab Try reading from the pipe to avoid warning on Windows
8d6f12a Use platform-specific @RootDir in File.expand_path spec
4e60d3d Avoid nonexistent pipe warning on Windows
24c26f9 Fix Dir.pwd encoding spec: it is the filesystem encoding.
e5406c1 Avoid newlines in File#open spec.
d21886d Disable autoclose if File.new(fd) does not raise an error
1f5ccb6 Fix namespace.
d7c8746 Take in account imprecision of Math.gamma on Windows.
ec58c97 Use a simpler way to know if Kernel has a private method with -n.
4f8ed56 Etc.group is not available on Windows
548e3c1 Avoid should_not raise_error.
40265a4 UNIXServer is not available on Windows
b0a70f1 Merge pull request #224 from ruby/vais/windows
4c24384 Pass Dir.mktmpdir spec on Windows
a0a0378 Adjust Socket constants and error classes for Windows
0d7762f not_supported_on is only for ruby implementations
9e87701 Adapt exception to Windows in recv_nonblock
bf0e472 Refactor IO#{read,write}_nonblock_spec when there is no data available
edaa5e7 Fix describe in Dir.mktmpdir spec
e6f91c7 IO#expect and the exepct stdlib is not available on Windows
3bb7369 Workaround the problem to test respond_to?(:fork) and being able to call it publicly.
e69370a Changing the time zone with Continent/City is unsupported on Windows
c81e224 Try to include private methods in the fork #respond_to? test
096cb4c Revert "Process.respond_to?(:fork) seems to be true now on Windows"
c6fcd2f Process.respond_to?(:fork) seems to be true now on Windows
c31500a Process.maxgroups is not on implemented on Windows
0a2ff59 Windows guards for symlinks
b5c2ed6 Guard UNIX-specific Signal.trap specs
d194a30 Windows guards in IO#write_nonblock
1f701d9 More IO on Windows fixes
774fb8f Only notify on status change for Travis
b8a8d3a Some IO on Windows fixes
c3c16bb More File on Windows fixes
618fa06 Fix File specs on Windows.
f5d45a0 Windows raises EISDIR for File.read(dir)
dbe045a Open files with write mode for writing in File#open spec
2ea1128 Add Windows guards in File.{new,open}
cd048ee Fix shared stat specs.
53b8dbc File.open with a directory is not supported on Windows.
d1a895e Skip incompatible Process#kill specs on Windows
6dd62b3 Separate Process#kill specs which can be supported on Windows
3dae14d Deduplicate definition of native newline.
130a11d Merge pull request #223 from ruby/vais/windows
86a2514 Pass Kernel#spawn spec on Windows
15b803c Pass Kernel#exec with a command array spec on Windows
e10c091 Merge pull request #222 from ruby/elia/explicit-fcall-as-default-arg
682e3c3 Use an ordinary Object instead of a String
18b9172 More explicit specs for fcall as default argument
f13db61 Merge pull request #221 from ruby/vais/windows
c716825 Pass Kernel#exec spec on Windows
80e5be3 Clearly separate non-compliant behavior on Windows using platform blocks
15c6b74 Pass Kernel#system spec on Windows
9049a75 Use a cross-platform alternative to system("false")
6fa70d1 Use a cross-platform alternative to system("true")
37677eb Pass IO.read spec on Windows
62e4e19 Pass IO#readlines spec on Windows
d84d68a Pass IO.foreach spec on Windows
1b2cdcf Fix grouping of #slice_when spec.

git-subtree-dir: spec/ruby
git-subtree-split: 32e22d667b12f10655701487312926f394d1e2e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants