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 UBSan error: Shifting 64 bit type by 64 is undefined behaviour #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanielG
Copy link

@DanielG DanielG commented Dec 18, 2019

Shifting a type by >=sizeof(type)*8 is undefined behaviour in C. Since
t->prng.buf is overwritten on the next iteration anyways this thankfully
isn't a real bug here.

Fixes #52

Shifting a type by >=sizeof(type)*8 is undefined behaviour in C. Since
t->prng.buf is overwritten on the next iteration anyways this thankfully
isn't a real bug here.

Fixes silentbicycle#52
@silentbicycle
Copy link
Owner

Being able to take more than 64 bits here could be a sign of a deeper issue. Thanks for reporting this.

I probably won't get to reviewing this closely for a bit because of the holidays, but when I do I'll retarget the PR for the develop branch.

@dcreager
Copy link

This just bit me too! I think the fix is correct — there's no attempt to take more than 64 bits, which you're right would be a deeper issue. It's just that if you take exactly 64 bits, then there's no need to shift anything in prng.buf, since you've just consumed it all, and will replace it with a new random uint64_t the next time through the loop. The if guard in this patch checks exactly that.

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.

Running with UBSan throws runtime error regarding bitshift
3 participants