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

TimeBasedEpochGenerator should prevent overflow #124

Open
chadparry opened this issue Dec 13, 2024 · 4 comments
Open

TimeBasedEpochGenerator should prevent overflow #124

chadparry opened this issue Dec 13, 2024 · 4 comments
Labels
pr-welcome Issue for which progress most likely if someone submits a Pull Request

Comments

@chadparry
Copy link

After monotonicity was added to the UUIDv7 implementation, ( 5563381 ), it became susceptible to rollover. That's a risk for any UUIDv7 implementation with a seeded monotonic counter. But most implementations try to mitigate the risk so that it won't happen unless a huge number of UUIDs are requested. With this implementation, the worst case is that it may fail even if only two UUIDs are requested within one time slice. The probability of that happening is 2-80, which makes it more of a theoretical risk. It is important enough to the specification that it actually warns against this though, ( https://www.rfc-editor.org/rfc/rfc9562#section-6.2-7.2 ). To be pedantically correct, the implementation could follow the specification's advice to zero out the first bit of randomness in the seed. That way the counter always starts out very far from an overflow.

Note that the carry logic doesn't take advantage of every single bit of entropy. Because it checks each byte against 0xff, the highest valid counter value is 0xfefefefefefefefefefe instead of 0xffffffffffffffffffff.


That could be corrected by comparing each byte to 0x00 instead, so that the carry bit gets handled just like in regular arithmetic.

@cowtowncoder
Copy link
Owner

Ok thank you for reporting the issue, @chadparry . Too bad I did not spot this in contribution; the original nanosecond timer does in fact guard against overflow.

If you have time and interest, a PR would be most welcome. But if not I hope I or someone else has time to address the issue.

@cowtowncoder cowtowncoder added the pr-welcome Issue for which progress most likely if someone submits a Pull Request label Dec 18, 2024
@chadparry
Copy link
Author

Hey, good to hear from you! I didn't know that was you until I saw your full name in the GitHub alert just now.

@cowtowncoder
Copy link
Owner

And I was about to ask if it was you (and not just name collision :) ).

Small world, indeed!

chadparry pushed a commit to chadparry/java-uuid-generator that referenced this issue Dec 26, 2024
@chadparry
Copy link
Author

If you have time and interest, a PR would be most welcome.

This bug isn't a big deal, but since it's your project, I can make time. :)

Note that the carry logic doesn't take advantage of every single bit of entropy. Because it checks each byte against 0xff, the highest valid counter value is 0xfefefefefefefefefefe instead of 0xffffffffffffffffffff.

I had originally read the code wrong; it used a temp variable to correctly check for 0xff before incrementing. However, I found a different counter bug. The UUIDUtil.constructUUID helper discards 6 entropy bits to make room for the version and variant. That means that duplicate UUIDs would get produced whenever the entropy differed only in those bits. Like the other bugs, this is more of a theoretical risk, since it's still possible to generate 262 UUIDs before the next 3×262 UUIDs would be duplicates. I decided to fix it so it wouldn't feel sloppy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-welcome Issue for which progress most likely if someone submits a Pull Request
Projects
None yet
Development

No branches or pull requests

2 participants