UnmarshalStream: Fixed two off-by-one errors in unmarshalInt #1338

Merged
merged 1 commit into from Dec 15, 2013

Conversation

Projects
None yet
2 participants
@DavidEGrayson
Contributor

DavidEGrayson commented Dec 15, 2013

While working on fixing issue #1329, I discovered something tangentially related that I think is a tiny bug and can be easily fixed. MRI's Marshal.load recognizes three different ways of encoding the integer 0 in a single byte, but JRuby 1.7.9 only handles one and raises the "marshal data too short" exception for the other two. This is because there are two off-by-one errors in JRuby's unmarshalInt method.

I think that unmarshalInt is supposed to be a clone of MRI's r_long function, so here is the source code of r_long:
https://github.com/ruby/ruby/blob/v2_0_0_352/marshal.c#L1114-1145

Here is the code I used to demonstrate the difference between MRI 2.0.0p0 and JRuby:

# MRI returns 0 for each, JRuby says marshal data is too short
puts Marshal.load("\x04\x08i\x05")
puts Marshal.load("\x04\x08i\xFB")

I suppose this isn't too important, because the number 0 will probably be marshaled as the 0x00 byte instead of 0xFB or 0x05, but if MRI treats those bytes as representing 0 then I don't think it is helpful for JRuby to raise an exception (or to try to construct an integer by reading 5 bytes from the stream).

I am happy to do any more investigating or documentation that is needed. Should I write a test for this? If so, where should I put it?

UnmarshalStream: Fixed two off-by-one errors in unmarshalInt; it is n…
…ow consistent with r_long in marshal.c of MRI. This also means there are three different ways to marshal the number 0.
@DavidEGrayson

This comment has been minimized.

Show comment
Hide comment
@DavidEGrayson

DavidEGrayson Dec 15, 2013

Contributor

The Travis CI build seems to have failed because it had trouble connecting to rubyforge.org port 443 and I don't think my change would have caused that.

Contributor

DavidEGrayson commented Dec 15, 2013

The Travis CI build seems to have failed because it had trouble connecting to rubyforge.org port 443 and I don't think my change would have caused that.

enebo added a commit that referenced this pull request Dec 15, 2013

Merge pull request #1338 from DavidEGrayson/jruby_pull_request_1
UnmarshalStream: Fixed two off-by-one errors in unmarshalInt

@enebo enebo merged commit cd01094 into jruby:jruby-1_7 Dec 15, 2013

1 check failed

default The Travis CI build failed
Details

@DavidEGrayson DavidEGrayson deleted the DavidEGrayson:jruby_pull_request_1 branch Jan 19, 2014

@enebo enebo modified the milestones: JRuby 1.7.10, JRuby 1.7.11 Feb 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment