src: fix freeing unintialized pointer bug in ParseSoaReply #35502
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested: |
nodejs-github-bot
added
c++
AasthaGupta
force-pushed
the
pointer-dealloc
branch
2 times, most recently
from
414fd5a
to
a23655a
Compare
|
Would it make sense to initialize the pointer variables with |
addaleax
approved these changes
Oct 4, 2020
LGTM either way, but I think initializing the pointers with nullptr instead might be a bit clearer :)
|
I totally agree with your comment @addaleax Let me also initialize the variable with nullptr :) |
|
@addaleax Is there anything else that needs to be done? |
|
@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 |
github-actions
bot
removed
the
request-ci
addaleax
added
author ready
Trott
approved these changes
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>
|
Landed in 0f41bca. Thanks for the contribution! |
BethGriggs
pushed a commit
that referenced
this issue
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>
Merged
MylesBorins
pushed a commit
that referenced
this issue
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>
Merged
MylesBorins
pushed a commit
that referenced
this issue
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 issue
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
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), orvcbuild test(Windows) passes