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.

Sign up for GitHub

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

Jump to bottom

src: fix integer overflow in GetNow #22214

Closed
wants to merge 1 commit into from
Closed

src: fix integer overflow in GetNow #22214

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Aug 9, 2018 •
edited
Loading

Well, that was stupid... This check used to be for 0xfffffff which was wrong (one f 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), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@apapirovski build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/477/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 9, 2018
addaleax
addaleax reviewed Aug 9, 2018
View reviewed changes
src/env.cc
@@ -617,7 +617,7 @@ Local<Value> Environment::GetNow() {
CHECK_GE(now, timer_base());
now -= timer_base();
if (now <= 0xffffffff)
Copy link
Member

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…?

Copy link
Member Author

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.

Copy link
Member

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. :)

@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/16298/

@ChALkeR
Copy link
Member

ChALkeR commented Aug 9, 2018 •
edited
Loading

This was introduced in #18486 and shouldn't affect versions prior to 10.0.0, correct?

@ChALkeR ChALkeR added dont-land-on-v6.x timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Aug 9, 2018
@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 10, 2018
@trivikr
Copy link
Member

trivikr commented Aug 12, 2018

Landed in 01a160a

@trivikr trivikr closed this Aug 12, 2018
trivikr pushed a commit that referenced this pull request Aug 12, 2018
@apapirovski @trivikr
src: fix integer overflow in GetNow
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>
targos pushed a commit that referenced this pull request Aug 12, 2018
@apapirovski @targos
src: fix integer overflow in GetNow
5b7419b 
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>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
@apapirovski apapirovski deleted the fix-integer-overflow-timers branch August 15, 2018 14:46
This was referenced Aug 16, 2018
Closed
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@calzoneman calzoneman mentioned this pull request Aug 21, 2018
Closed
@addaleax addaleax mentioned this pull request Aug 31, 2018
Closed
This was referenced Aug 31, 2018
Closed
Open
@ziyunfei ziyunfei mentioned this pull request Sep 10, 2018
Closed
nkbt
nkbt reviewed Oct 9, 2018
View reviewed changes
src/env.cc
@@ -617,7 +617,7 @@ Local<Value> Environment::GetNow() {
CHECK_GE(now, timer_base());
now -= timer_base();
if (now <= 0xffffffff)
Copy link

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?

Copy link
Member

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

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@nkbt nkbt nkbt left review comments

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mmarchini mmarchini mmarchini approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@bmeck bmeck bmeck approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

setInterval callback function unexpected halt