gh-138862: fix timestamp increment after UUIDv7 counter overflow#146606
gh-138862: fix timestamp increment after UUIDv7 counter overflow#146606picnixz wants to merge 10 commits intopython:mainfrom
Conversation
|
I'll need to add a test for multiple overflows to see that I only increment on overflow. |
|
@hugovk @eendebakpt Could you have a look at this? TL;DR: it shouldn't happen in practice because the counter is a randomized 41-bit integer and its max value is From a practical PoV, So, for a realistic attack, (or issue) one needs one of the following situations to happen:
|
| # after a counter overflow. We follow the RFC for in the former | ||
| # case. In the latter case, we re-use the already advanced | ||
| # timestamp (it was updated when we detected the overflow). | ||
| if _last_counter_v7_overflow: |
There was a problem hiding this comment.
The first case from the if-else is the "latter" case in the comments above. Maybe swap the order in the comments.
There was a problem hiding this comment.
Oh right :') I changed so much this block that I forgot to update the comment. Thanks!
| nanoseconds = time.time_ns() | ||
| timestamp_ms = nanoseconds // 1_000_000 | ||
|
|
||
| if _last_timestamp_v7 is None or timestamp_ms > _last_timestamp_v7: |
There was a problem hiding this comment.
Side note: if we initialize _last_timestamp_v7 to a large negative value (e.g. -2**64), then we can remove the last_timestamp_v7 is None check.
There was a problem hiding this comment.
Or just -1 and it's ok. You won't have a negative timestamp I think.
There was a problem hiding this comment.
You could (according to Claude when setting the system time to before 1970). But since the timestamp is generated via a int64_t the value -2**64 should be safe.
|
@picnixz I agree with your analysis. Whether or not to merge this PR (it fixes the bug) or leave as is (this will not happen in practice) I am 50/50 on. |
|
I don't think we are loosing much speed. We are only adding one comparison every time we are within the same millisecond. But this adds a bit complexity that we will need to replicate in C though not necessarily hard to do. But still, it's a bit annoying I think. I can also document the implementation more. |
Uh oh!
There was an error while loading. Please reload this page.