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

fs: fix memory leak in WriteString #19357

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 30 additions & 13 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1356,7 +1356,6 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {

const auto enc = ParseEncoding(env->isolate(), args[3], UTF8);

std::unique_ptr<char[]> delete_on_return;
Local<Value> value = args[1];
char* buf = nullptr;
size_t len;
Expand Down Expand Up @@ -1384,24 +1383,42 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
}
}

if (buf == nullptr) {
if (is_async) { // write(fd, string, pos, enc, req)
CHECK_NE(req_wrap, nullptr);
len = StringBytes::StorageSize(env->isolate(), value, enc);
buf = new char[len];
// SYNC_CALL returns on error. Make sure to always free the memory.
if (!is_async) delete_on_return.reset(buf);
FSReqBase::FSReqBuffer& stack_buffer =
req_wrap->Init("write", len, enc);
// 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);
}

uv_buf_t uvbuf = uv_buf_init(buf, len);

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);
Copy link
Member Author

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
Member

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
Member Author

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);
req_wrap->Dispatched();
if (err < 0) {
uv_fs_t* uv_req = req_wrap->req();
uv_req->result = err;
uv_req->path = nullptr;
AfterInteger(uv_req); // after may delete req_wrap if there is an error
} else {
req_wrap->SetReturnValue(args);
}
} else { // write(fd, string, pos, enc, undefined, ctx)
CHECK_EQ(argc, 6);
fs_req_wrap req_wrap;
FSReqBase::FSReqBuffer stack_buffer;
if (buf == nullptr) {
len = StringBytes::StorageSize(env->isolate(), value, enc);
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(), *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",
uv_fs_write, fd, &uvbuf, 1, pos);
args.GetReturnValue().Set(bytesWritten);
Expand Down
20 changes: 16 additions & 4 deletions src/node_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ namespace fs {

class FSReqBase : public ReqWrap<uv_fs_t> {
public:
typedef MaybeStackBuffer<char, 64> FSReqBuffer;

FSReqBase(Environment* env, Local<Object> req, AsyncWrap::ProviderType type)
: ReqWrap(env, req, type) {
Wrap(object(), this);
Expand All @@ -32,9 +34,9 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
}

void Init(const char* syscall,
const char* data = nullptr,
size_t len = 0,
enum encoding encoding = UTF8) {
const char* data,
size_t len,
enum encoding encoding) {
syscall_ = syscall;
encoding_ = encoding;

Expand All @@ -47,6 +49,16 @@ class FSReqBase : public ReqWrap<uv_fs_t> {
}
}

FSReqBuffer& Init(const char* syscall, size_t len,
enum encoding encoding) {
syscall_ = syscall;
encoding_ = encoding;

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

virtual void FillStatsArray(const uv_stat_t* stat) = 0;
virtual void Reject(Local<Value> reject) = 0;
virtual void Resolve(Local<Value> value) = 0;
Expand All @@ -66,7 +78,7 @@ class FSReqBase : public ReqWrap<uv_fs_t> {

// Typically, the content of buffer_ is something like a file name, so
// something around 64 bytes should be enough.
MaybeStackBuffer<char, 64> buffer_;
FSReqBuffer buffer_;

DISALLOW_COPY_AND_ASSIGN(FSReqBase);
};
Expand Down