-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
src: fix integer overflow in GetNow #22214
Conversation
@@ -617,7 +617,7 @@ Local<Value> Environment::GetNow() { | |||
CHECK_GE(now, timer_base()); | |||
now -= timer_base(); | |||
if (now <= 0xffffffff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have an U
suffix…?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't matter in this case, right? I thought they were equivalent since this won't fit into an int
and the next type is unsigned int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I looked it up – without the suffix it is still signed, but it’s a long int
in that case, so we’re probably fine here. :)
This was introduced in #18486 and shouldn't affect versions prior to 10.0.0, correct? |
Landed in 01a160a |
PR-URL: #22214 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: #22214 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@@ -617,7 +617,7 @@ Local<Value> Environment::GetNow() { | |||
CHECK_GE(now, timer_base()); | |||
now -= timer_base(); | |||
if (now <= 0xffffffff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the bug still exists for numbers in 2^31 .. 2^32
range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkbt This PR fixed the bug for those numbers – all others should remain the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was not sure if NewFromUnsigned does exactly that
Well, that was stupid... This check used to be for
0xfffffff
which was wrong (onef
too few) so that got fixed, but that change was made based on it being cast to a uint32_t (2**32-1) rather than the correct int32_t. Of course,Integer::New
just happily accepted it despite not actually being able to handle it. Fun stuff...Fixes #22149
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes