src: fix freeing unintialized pointer bug in ParseSoaReply - #35502
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
LGTM either way, but I think initializing the pointers with nullptr instead might be a bit clearer :)
Sorry, something went wrong.
|
I totally agree with your comment @addaleax Let me also initialize the variable with nullptr :) |
Sorry, something went wrong.
|
@addaleax Is there anything else that needs to be done? |
Sorry, something went wrong.
|
@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. :)) |
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/33396/ (:green_heart:) |
Sorry, something went wrong.
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! 🎉 |
Sorry, something went wrong.
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>
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>
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>
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>
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