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

fix(ruby): Fix crash when calculating Message hash values on 64-bit Windows #8565

Merged
merged 5 commits into from May 6, 2021

Conversation

dazuma
Copy link
Contributor

@dazuma dazuma commented May 5, 2021

Fixes #8564

On 64-bit Windows, it is unsafe to try to create a Fixnum from a 64-bit integer, because longs can hold only 32 bits. The code change below:

  • Downcasts the original unsigned 64-bit hash to a signed long (which may be either 64 bits or 32 bits depending on the platform).
  • Checks the value to see if it's in the half of the range of long that corresponds to a valid Fixnum, and moves it into range if not.
  • Only then does it pass the result into INT2FIX.

/attn @haberman

@google-cla google-cla bot added the cla: yes label May 5, 2021
@dazuma
Copy link
Contributor Author

dazuma commented May 5, 2021

Updated with a uniform mapping.

return INT2FIX(Message_Hash(self->msg, self->msgdef, 0));
uint64_t hash_value = Message_Hash(self->msg, self->msgdef, 0);
// Fixnum is restricted to signed long, so downcast if necessary.
long long_value = (long)hash_value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

  // RUBY_FIXNUM_MAX should be one less than a power of 2.
  assert(RUBY_FIXNUM_MAX & (RUBY_FIXNUM_MAX + 1) == 0);  
  return INT2FIX(hash_value & RUBY_FIXNUM_MAX);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that works. It restricts hash values to positive, but I think 32-bit hashes are always positive for most Ruby objects anyway.

@dazuma
Copy link
Contributor Author

dazuma commented May 6, 2021

Looking at the failed Ruby 2.4 test, I think we can address it by pinning the irb gem to ~> 1.1 on Ruby 2.4 (since newer versions of irb depend on reline, which requires Ruby 2.5+).

return INT2FIX(Message_Hash(self->msg, self->msgdef, 0));
uint64_t hash_value = Message_Hash(self->msg, self->msgdef, 0);
// RUBY_FIXNUM_MAX should be one less than a power of 2.
assert(RUBY_FIXNUM_MAX & (RUBY_FIXNUM_MAX + 1) == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is my bad, I always forget that the precedence of & is low. It should be:

assert((RUBY_FIXNUM_MAX & (RUBY_FIXNUM_MAX + 1)) == 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I forgot that too. Fixed.

@haberman
Copy link
Member

haberman commented May 6, 2021

I think we can address it by pinning the irb gem to ~> 1.1 on Ruby 2.4 (since newer versions of irb depend on reline, which requires Ruby 2.5+).

SGTM.

@dazuma
Copy link
Contributor Author

dazuma commented May 6, 2021

Looking at the failed Ruby 2.4 test, I think we can address it by pinning the irb gem to ~> 1.1 on Ruby 2.4 (since newer versions of irb depend on reline, which requires Ruby 2.5+).

I added this change to the Gemfile. We can try rerunning the tests to make sure it works.

@haberman haberman merged commit 7e3bbed into protocolbuffers:master May 6, 2021
@dazuma dazuma deleted the pr/message-hash branch May 6, 2021 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ruby] Message#hash crashes with a C assertion on Windows
3 participants