Skip to content
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

Ruby hash broken for Messages with repeated fields. #2036

Closed
abscondment opened this issue Aug 30, 2016 · 0 comments
Closed

Ruby hash broken for Messages with repeated fields. #2036

abscondment opened this issue Aug 30, 2016 · 0 comments

Comments

@abscondment
Copy link
Contributor

abscondment commented Aug 30, 2016

Expected

All Ruby Message objects return a hash.

Actual

Calling hash on a Message with a repeated field will occasionally raise the following error:

RangeError: bignum too big to convert into `long'

Test Case

I have written a gist with tests demonstrating the behavior.

Details

This is because of a bug in repeated_field.c, wherein the hash function occasionally returns a Bignum instead of a Fixnum. RepeatedField_hash can return a Bignum because of the algorithm it uses for combining hashes: bitwise shift left, then xor. If the original hash is large enough, the << will cause Ruby to automagically promote the value from a Fixnum to a Bignum.

Ruby indicates that hash should return a Fixnum, so this behavior is not correct.

Up the stack, storage.c attempts to convert this Bignum to a long, which produces the RangeError.

Proposal

I think the correct behavior is to use rb_hash_start -> rb_hash_uint -> rb_hash_end -> INT2FIX as storage.c does. I'd be happy to provide a patch for this if that sounds correct. I've provided a PR.

abscondment added a commit to abscondment/protobuf that referenced this issue Aug 30, 2016
* add a repeated field to the tested hash
* also assert that two messages with identical values produce identical hashes
haberman added a commit that referenced this issue Sep 19, 2016
Fix #2036 (Ruby `hash` broken for Messages with repeated fields.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants