From ffca2fec0a0de295967ca27da8d956ab6b9337fb Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 15 Mar 2018 04:47:35 +0800 Subject: [PATCH 1/2] fs: fix memory leak in WriteString 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: https://github.com/nodejs/node/issues/19356 --- src/node_file.cc | 42 +++++++++++++++++++++++++++++------------- src/node_file.h | 21 +++++++++++++++++---- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index fe3b0e1383e8cb..daf817307caf7c 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1356,7 +1356,6 @@ static void WriteString(const FunctionCallbackInfo& args) { const auto enc = ParseEncoding(env->isolate(), args[3], UTF8); - std::unique_ptr delete_on_return; Local value = args[1]; char* buf = nullptr; size_t len; @@ -1384,24 +1383,41 @@ static void WriteString(const FunctionCallbackInfo& 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); + 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 + 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 delete_on_return; + 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); + // 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); int bytesWritten = SyncCall(env, args[5], &req_wrap, "write", uv_fs_write, fd, &uvbuf, 1, pos); args.GetReturnValue().Set(bytesWritten); diff --git a/src/node_file.h b/src/node_file.h index bf277a0e433525..0b1b3981c4be90 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -22,6 +22,8 @@ namespace fs { class FSReqBase : public ReqWrap { public: + typedef MaybeStackBuffer FSReqBuffer; + FSReqBase(Environment* env, Local req, AsyncWrap::ProviderType type) : ReqWrap(env, req, type) { Wrap(object(), this); @@ -32,9 +34,9 @@ class FSReqBase : public ReqWrap { } 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; @@ -47,6 +49,17 @@ class FSReqBase : public ReqWrap { } } + FSReqBuffer& Init(const char* syscall, size_t len, + enum encoding encoding) { + syscall_ = syscall; + 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_; + } + virtual void FillStatsArray(const uv_stat_t* stat) = 0; virtual void Reject(Local reject) = 0; virtual void Resolve(Local value) = 0; @@ -66,7 +79,7 @@ class FSReqBase : public ReqWrap { // Typically, the content of buffer_ is something like a file name, so // something around 64 bytes should be enough. - MaybeStackBuffer buffer_; + FSReqBuffer buffer_; DISALLOW_COPY_AND_ASSIGN(FSReqBase); }; From ebd7b7d3df19005f1196f0885f008c242aa4645c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 15 Mar 2018 05:55:50 +0800 Subject: [PATCH 2/2] squash! call SetLengthAndZeroTerminate and use MaybeStackBuffer in the sync case --- src/node_file.cc | 13 +++++++------ src/node_file.h | 1 - 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index daf817307caf7c..e1cae44a8a51e4 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1391,6 +1391,7 @@ static void WriteString(const FunctionCallbackInfo& 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); + 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); @@ -1400,22 +1401,22 @@ static void WriteString(const FunctionCallbackInfo& 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 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", diff --git a/src/node_file.h b/src/node_file.h index 0b1b3981c4be90..9e9001ddd7322c 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -55,7 +55,6 @@ class FSReqBase : public ReqWrap { 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_; }