Skip to content

fs: fix memory leak in WriteString #19357

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.

Sign up for GitHub

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

Closed
joyeecheung wants to merge 2 commits into from
Closed

fs: fix memory leak in WriteString #19357

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffca2fe
fs: fix memory leak in WriteString
joyeecheung Mar 14, 2018
ebd7b7d
squash! call SetLengthAndZeroTerminate and use MaybeStackBuffer in th…
joyeecheung Mar 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
squash! call SetLengthAndZeroTerminate and use MaybeStackBuffer in th… 
…e sync case
  • Loading branch information
@joyeecheung
joyeecheung committed Mar 14, 2018
commit ebd7b7d3df19005f1196f0885f008c242aa4645c
13 changes: 7 additions & 6 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,6 +1391,7 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
// StorageSize may return too large a char, so correct the actual length
// by the write size
len = StringBytes::Write(env->isolate(), *stack_buffer, len, args[1], enc);
Copy link
Copy Markdown
Member Author

@joyeecheung joyeecheung Mar 14, 2018

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?

Copy link
Copy Markdown
Member

@addaleax addaleax Mar 14, 2018

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?

Copy link
Copy Markdown
Member Author

@joyeecheung joyeecheung Mar 14, 2018

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..

stack_buffer.SetLengthAndZeroTerminate(len);
uv_buf_t uvbuf = uv_buf_init(*stack_buffer, len);
int err = uv_fs_write(env->event_loop(), req_wrap->req(),
fd, &uvbuf, 1, pos, AfterInteger);
Expand All @@ -1400,22 +1401,22 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
uv_req->result = err;
uv_req->path = nullptr;
AfterInteger(uv_req); // after may delete req_wrap if there is an error
req_wrap = nullptr;
} else {
req_wrap->SetReturnValue(args);
}
} else { // write(fd, string, pos, enc, undefined, ctx)
CHECK_EQ(argc, 6);
fs_req_wrap req_wrap;
std::unique_ptr<char[]> delete_on_return;
FSReqBase::FSReqBuffer stack_buffer;
if (buf == nullptr) {
len = StringBytes::StorageSize(env->isolate(), value, enc);
buf = new char[len];
// Make sure to always free the memory.
delete_on_return.reset(buf);
stack_buffer.AllocateSufficientStorage(len + 1);
// StorageSize may return too large a char, so correct the actual length
// by the write size
len = StringBytes::Write(env->isolate(), buf, len, args[1], enc);
len = StringBytes::Write(env->isolate(), *stack_buffer,
len, args[1], enc);
stack_buffer.SetLengthAndZeroTerminate(len);
buf = *stack_buffer;
}
uv_buf_t uvbuf = uv_buf_init(buf, len);
int bytesWritten = SyncCall(env, args[5], &req_wrap, "write",
Expand Down
1 change: 0 additions & 1 deletion src/node_file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
encoding_ = encoding;

buffer_.AllocateSufficientStorage(len + 1);
buffer_.SetLengthAndZeroTerminate(len);
has_data_ = false; // so that the data does not show up in error messages
return buffer_;
}
Expand Down