Skip to content

Fix UB when hashing some doubles in 32bits. #1680

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

Merged

Conversation

drodriguez
Copy link
Contributor

In 32 bits platforms, for some doubles the hash result could be
incorrectly zero (or whatever undefined behaviour happens to be).

The problem happens when the non fractional part of the double is high
and the fractional part is between 0 and 0.5 and high enough to make the
sum of both not fit into 32 bits. In that case, the use of += promotes
result to double to add it to the fractional double, but when it gets
converted back into CFHashCode, since it is outside the 32 bit range, it
might end up as zero.

Casting fractional to CFHashCode avoids the hidden promotion, and should
not fail, since fractional is at most ULONG_MAX/2.

The problem doesn't exist in 64 bits, since CFHashCode is 64 bits wide
there.

In 32 bits platforms, for some doubles the hash result could be
incorrectly zero (or whatever undefined behaviour happens to be).

The problem happens when the non fractional part of the double is high
and the fractional part is between 0 and 0.5 and high enough to make the
sum of both not fit into 32 bits. In that case, the use of += promotes
result to double to add it to the fractional double, but when it gets
converted back into CFHashCode, since it is outside the 32 bit range, it
might end up as zero.

Casting fractional to CFHashCode avoids the hidden promotion, and should
not fail, since fractional is at most ULONG_MAX/2.

The problem doesn't exist in 64 bits, since CFHashCode is 64 bits wide
there.
@drodriguez
Copy link
Contributor Author

This is hard to check. Android might be the easiest. With the latest Xcode I don’t see a way of building 32 bits macOS, and Linux doesn’t seem to support 32 bits. If someone needs more explanations I can provide them.

I found this because I was hashing dates and comparing that the hash wasn’t zero (which is not failproof, but in this case the result was actually incorrect). I was having sporadic failures in 32 bits, but not 64 bits, which seemed odd. I found one value that triggered the problem and I found stepping through the assembly code where the problem was.

@drodriguez
Copy link
Contributor Author

@swift-ci test

1 similar comment
@spevans
Copy link
Contributor

spevans commented Aug 31, 2018

@swift-ci test

@millenomi millenomi merged commit 9f217d1 into swiftlang:master Aug 31, 2018
@drodriguez drodriguez deleted the fix-CFHashDouble-undefined-behaviour branch July 16, 2019 22:37
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.

3 participants