From 897cec43c67d036f631e0538a8172a5a8f759611 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Thu, 15 Mar 2018 04:47:35 +0800
Subject: [PATCH] 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

PR-URL: https://github.com/nodejs/node/pull/19357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/node_file.cc | 43 ++++++++++++++++++++++++++++++-------------
 src/node_file.h  | 20 ++++++++++++++++----
 2 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/src/node_file.cc b/src/node_file.cc
index 36bb326aa51..510689607e8 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -1527,7 +1527,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;
@@ -1555,24 +1554,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);
+    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);
diff --git a/src/node_file.h b/src/node_file.h
index fa373d46ad0..1925e400f2d 100644
--- a/src/node_file.h
+++ b/src/node_file.h
@@ -24,6 +24,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);
@@ -34,9 +36,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;
 
@@ -49,6 +51,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;
@@ -68,7 +80,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);
 };