fix(ruby): Fix crash when calculating Message hash values on 64-bit Windows#8565
fix(ruby): Fix crash when calculating Message hash values on 64-bit Windows#8565haberman merged 5 commits intoprotocolbuffers:masterfrom
Conversation
|
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; |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Yeah, that works. It restricts hash values to positive, but I think 32-bit hashes are always positive for most Ruby objects anyway.
|
Looking at the failed Ruby 2.4 test, I think we can address it by pinning the |
| 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); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Whoops, I forgot that too. Fixed.
SGTM. |
I added this change to the Gemfile. We can try rerunning the tests to make sure it works. |
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:
INT2FIX./attn @haberman