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

potential null pointer deref? #19991

Closed
deian opened this issue Apr 12, 2018 · 4 comments · Fixed by #21545
Closed

potential null pointer deref? #19991

deian opened this issue Apr 12, 2018 · 4 comments · Fixed by #21545
Labels
trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.

Comments

@deian
Copy link
Member

deian commented Apr 12, 2018

We noticed these two sites as potential null pointer deferences.

const uint8_t* category_group_enabled =
GetCategoryGroupEnabled(category_group);
if (*category_group_enabled == 0) return;

const uint8_t* category_group_enabled =
GetCategoryGroupEnabled(category_group);
args.GetReturnValue().Set(*category_group_enabled > 0);

It seems like category_group_enabled can be null based on the defensive check:

static const uint8_t* GetCategoryGroupEnabled(const char* category_group) {
if (category_group == nullptr) return nullptr;
return TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(category_group);
}

But I suspect because of the usage GetCategoryGroupEnabled is not called with null pointer. Any thoughts?

@jasnell
Copy link
Member

jasnell commented Apr 12, 2018

/cc @AndreasMadsen @addaleax

@jasnell jasnell added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Apr 12, 2018
@addaleax
Copy link
Member

Yeah, the value passed to GetCategoryGroupEnabled() is never a null pointer because it’s the return value of GetCategoryGroup(). I don’t think there’s anything we need to do here?

@AndreasMadsen
Copy link
Member

We could add a check that category_group isn't nullptr in GetCategoryGroupEnabled.

@deian
Copy link
Member Author

deian commented Apr 13, 2018

Either way! I wasn't going to create this issue, but the defensive check on line 20 that checks for nullptr made it seem like something worth double checking. Thanks for your responses!

cjihrig added a commit to cjihrig/node that referenced this issue Jun 28, 2018
The input to this function shouldn't be null, and callers are
not equipped to deal with a nullptr return value. Change the
nullptr return to a CHECK_NOT_NULL(). Also fix the indentation
of the function.

PR-URL: nodejs#21545
Fixes: nodejs#19991
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this issue Jun 28, 2018
The input to this function shouldn't be null, and callers are
not equipped to deal with a nullptr return value. Change the
nullptr return to a CHECK_NOT_NULL(). Also fix the indentation
of the function.

PR-URL: #21545
Fixes: #19991
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@deian @jasnell @AndreasMadsen @addaleax and others