-
Notifications
You must be signed in to change notification settings - Fork 29k
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
fs: fix memory leak in WriteString #19357
Conversation
In the async case, if the buffer was copied instead of being moved then the buf will not get deleted after the request is done. This was introduced when the FSReqWrap:: Ownership was removed in 4b9ba9b, and ReleaseEarly was no longer called upon destruction of FSReqWrap. Create a custom Init function so we can use the MaybeStackBuffer in the FSReqBase to copy the string in the async case. The data will then get destructed when the FSReqBase is destructed. Fixes: nodejs#19356
src/node_file.cc
Outdated
} else { // write(fd, string, pos, enc, undefined, ctx) | ||
CHECK_EQ(argc, 6); | ||
fs_req_wrap req_wrap; | ||
std::unique_ptr<char[]> delete_on_return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, this could also very well be a MaybeStackBuffer
. It’s more or less what they are there for :)
if (is_async) { // write(fd, string, pos, enc, req) | ||
AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger, | ||
uv_fs_write, fd, &uvbuf, 1, pos); | ||
len = StringBytes::Write(env->isolate(), *stack_buffer, len, args[1], enc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does StringBytes::Write
guarantees null termination? From the test results I guess it does, so there is no need to call SetLengthAndZeroTerminate
again after this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
StringBytes::Write
guarantees null termination?
I don’t think so … and it actually specifies NO_NULL_TERMINATION
for V8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax OK, I will just call SetLengthAndZeroTerminate
to be safe..
Updated to call |
Landed in 897cec4, thanks! edit: edited by @MylesBorins to fix commit sha |
In the async case, if the buffer was copied instead of being moved then the buf will not get deleted after the request is done. This was introduced when the FSReqWrap:: Ownership was removed in 4b9ba9b, and ReleaseEarly was no longer called upon destruction of FSReqWrap. Create a custom Init function so we can use the MaybeStackBuffer in the FSReqBase to copy the string in the async case. The data will then get destructed when the FSReqBase is destructed. Fixes: #19356 PR-URL: #19357 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Should this be backported to |
@MylesBorins This fixed a leak introduced in #18112 so depends on whether that will be backported. |
In the async case, if the buffer was copied instead of being moved
then the buf will not get deleted after the request is done.
This was introduced when the FSReqWrap:: Ownership was removed in 4b9ba9b,
and ReleaseEarly was no longer called upon destruction of FSReqWrap.
Create a custom Init function so we can use the MaybeStackBuffer in
the FSReqBase to copy the string in the async case. The data will
then get destructed when the FSReqBase is destructed.
Fixes: #19356
Before this patch, running
valgrind --leak-check=yes ./node --expose_externalize_string test/parallel/test-fs-write.js
produces:After this patch, this leak is gone.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes