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

src: fix freeing unintialized pointer bug in ParseSoaReply #35502

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

AasthaGupta
Copy link
Contributor

@AasthaGupta AasthaGupta commented Oct 4, 2020

ares_expand_name doesn't guarantee that pointer variable is initialized if
return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the function
in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even though
it is a local variable and we create a unique pointer soon after calling
ares_expand_name. This could potentially crash
the program with an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

node(9118,0x111471dc0) malloc: *** error for object 0x7b: pointer being freed was not allocated
node(9118,0x111471dc0) malloc: *** set a breakpoint in malloc_error_break to debug
[1] 9118 abort node ../temp.js

By moving the unique_ptr after checking the return code we can fix the problem.
As the underlying function guarantees that pointer is initialized when the
status is ARES_SUCCESS.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@AasthaGupta AasthaGupta requested a review from a team as a code owner October 4, 2020 15:05
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Oct 4, 2020
@AasthaGupta AasthaGupta force-pushed the pointer-dealloc branch 2 times, most recently from 414fd5a to a23655a Compare October 4, 2020 15:16
@addaleax
Copy link
Member

addaleax commented Oct 4, 2020

Would it make sense to initialize the pointer variables with nullptr instead? That way the code is a bit more “obviously correct”, because then it’s clear (without checking the c-ares API documentation ) that 1. we never leak memory, because unique_ptr always takes ownership, and 2. we never access uninitialized memory, because if the call fails, the unique_ptr will just be empty.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM either way, but I think initializing the pointers with nullptr instead might be a bit clearer :)

@AasthaGupta
Copy link
Contributor Author

I totally agree with your comment @addaleax

Let me also initialize the variable with nullptr :)

@AasthaGupta
Copy link
Contributor Author

@addaleax Is there anything else that needs to be done?

@addaleax
Copy link
Member

addaleax commented Oct 5, 2020

@AasthaGupta We currently have a 48-hour waiting period before PRs are merged, so that’s why this isn’t merged yet. :)

(Btw, my original suggestion here was to initialize with nullptr instead of moving the unique_ptr construction sites, not in addition to – that way it would be clear that any memory allocated by c-ares would always be released without knowing how c-ares works, because even if c-ares did allocate and store data, that would be okay then. But since that’s not what’s currently happening in practice, this isn’t really important to me. :))

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 5, 2020

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Oct 5, 2020
ares_expand_name doesn't guarantee that pointer variable is initialized
if return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the
function in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even
though it is a local variable and we create a unique pointer soon after
calling ares_expand_name. This could potentially crash the program with
an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

By moving the unique_ptr after checking the return code we can fix the
problem. As the underlying function guarantees that pointer is
initialized when the status is ARES_SUCCESS.

PR-URL: nodejs#35502
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott merged commit 0f41bca into nodejs:master Oct 8, 2020
@Trott
Copy link
Member

Trott commented Oct 8, 2020

Landed in 0f41bca.

Thanks for the contribution! 🎉

BethGriggs pushed a commit that referenced this pull request Oct 13, 2020
ares_expand_name doesn't guarantee that pointer variable is initialized
if return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the
function in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even
though it is a local variable and we create a unique pointer soon after
calling ares_expand_name. This could potentially crash the program with
an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

By moving the unique_ptr after checking the return code we can fix the
problem. As the underlying function guarantees that pointer is
initialized when the status is ARES_SUCCESS.

PR-URL: #35502
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 14, 2020
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
ares_expand_name doesn't guarantee that pointer variable is initialized
if return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the
function in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even
though it is a local variable and we create a unique pointer soon after
calling ares_expand_name. This could potentially crash the program with
an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

By moving the unique_ptr after checking the return code we can fix the
problem. As the underlying function guarantees that pointer is
initialized when the status is ARES_SUCCESS.

PR-URL: #35502
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
ares_expand_name doesn't guarantee that pointer variable is initialized
if return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the
function in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even
though it is a local variable and we create a unique pointer soon after
calling ares_expand_name. This could potentially crash the program with
an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

By moving the unique_ptr after checking the return code we can fix the
problem. As the underlying function guarantees that pointer is
initialized when the status is ARES_SUCCESS.

PR-URL: #35502
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
ares_expand_name doesn't guarantee that pointer variable is initialized
if return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the
function in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even
though it is a local variable and we create a unique pointer soon after
calling ares_expand_name. This could potentially crash the program with
an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

By moving the unique_ptr after checking the return code we can fix the
problem. As the underlying function guarantees that pointer is
initialized when the status is ARES_SUCCESS.

PR-URL: nodejs#35502
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants