src: use RAII for uv_process_options_t

iknoom · iknoom · commit 13fe1b56c4b4 · 2025-09-20T20:52:14.000+09:00
diff --git a/src/process_wrap.cc b/src/process_wrap.cc
@@ -116,9 +116,10 @@ class ProcessWrap : public HandleWrap {
     return Just(stream);
   }
 
-  static Maybe<void> ParseStdioOptions(Environment* env,
-                                       Local<Object> js_options,
-                                       uv_process_options_t* options) {
+  static Maybe<void> ParseStdioOptions(
+      Environment* env,
+      Local<Object> js_options,
+      std::vector<uv_stdio_container_t>* options_stdio) {
     Local<Context> context = env->context();
     Local<String> stdio_key = env->stdio_string();
     Local<Value> stdios_val;
@@ -132,8 +133,7 @@ class ProcessWrap : public HandleWrap {
     Local<Array> stdios = stdios_val.As<Array>();
 
     uint32_t len = stdios->Length();
-    options->stdio = new uv_stdio_container_t[len];
-    options->stdio_count = len;
+    options_stdio->resize(len);
 
     for (uint32_t i = 0; i < len; i++) {
       Local<Value> val;
@@ -147,23 +147,23 @@ class ProcessWrap : public HandleWrap {
       }
 
       if (type->StrictEquals(env->ignore_string())) {
-        options->stdio[i].flags = UV_IGNORE;
+        (*options_stdio)[i].flags = UV_IGNORE;
       } else if (type->StrictEquals(env->pipe_string())) {
-        options->stdio[i].flags = static_cast<uv_stdio_flags>(
+        (*options_stdio)[i].flags = static_cast<uv_stdio_flags>(
             UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE);
-        if (!StreamForWrap(env, stdio).To(&options->stdio[i].data.stream)) {
+        if (!StreamForWrap(env, stdio).To(&(*options_stdio)[i].data.stream)) {
           return Nothing<void>();
         }
       } else if (type->StrictEquals(env->overlapped_string())) {
-        options->stdio[i].flags = static_cast<uv_stdio_flags>(
-            UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE |
-            UV_OVERLAPPED_PIPE);
-        if (!StreamForWrap(env, stdio).To(&options->stdio[i].data.stream)) {
+        (*options_stdio)[i].flags =
+            static_cast<uv_stdio_flags>(UV_CREATE_PIPE | UV_READABLE_PIPE |
+                                        UV_WRITABLE_PIPE | UV_OVERLAPPED_PIPE);
+        if (!StreamForWrap(env, stdio).To(&(*options_stdio)[i].data.stream)) {
           return Nothing<void>();
         }
       } else if (type->StrictEquals(env->wrap_string())) {
-        options->stdio[i].flags = UV_INHERIT_STREAM;
-        if (!StreamForWrap(env, stdio).To(&options->stdio[i].data.stream)) {
+        (*options_stdio)[i].flags = UV_INHERIT_STREAM;
+        if (!StreamForWrap(env, stdio).To(&(*options_stdio)[i].data.stream)) {
           return Nothing<void>();
         }
       } else {
@@ -174,8 +174,8 @@ class ProcessWrap : public HandleWrap {
         }
         CHECK(fd_value->IsNumber());
         int fd = FromV8Value<int>(fd_value);
-        options->stdio[i].flags = UV_INHERIT_FD;
-        options->stdio[i].data.fd = fd;
+        (*options_stdio)[i].flags = UV_INHERIT_FD;
+        (*options_stdio)[i].data.fd = fd;
       }
     }
     return JustVoid();
@@ -199,8 +199,6 @@ class ProcessWrap : public HandleWrap {
 
     options.exit_cb = OnExit;
 
-    // TODO(bnoordhuis) is this possible to do without mallocing ?
-
     // options.file
     Local<Value> file_v;
     if (!js_options->Get(context, env->file_string()).ToLocal(&file_v)) {
@@ -251,23 +249,26 @@ class ProcessWrap : public HandleWrap {
     if (!js_options->Get(context, env->args_string()).ToLocal(&argv_v)) {
       return;
     }
-    if (!argv_v.IsEmpty() && argv_v->IsArray()) {
+    std::vector<char*> options_args;
+    std::vector<std::string> args_vals;
+    if (argv_v->IsArray()) {
       Local<Array> js_argv = argv_v.As<Array>();
       int argc = js_argv->Length();
       CHECK_LT(argc, INT_MAX);  // Check for overflow.
-
-      // Heap allocate to detect errors. +1 is for nullptr.
-      options.args = new char*[argc + 1];
+      args_vals.reserve(argc);
+      options_args.resize(argc + 1);
       for (int i = 0; i < argc; i++) {
         Local<Value> val;
         if (!js_argv->Get(context, i).ToLocal(&val)) {
           return;
         }
         node::Utf8Value arg(env->isolate(), val);
-        options.args[i] = strdup(*arg);
-        CHECK_NOT_NULL(options.args[i]);
+        args_vals.emplace_back(arg.ToString());
+        options_args[i] = const_cast<char*>(args_vals.back().c_str());
+        CHECK_NOT_NULL(options_args[i]);
       }
-      options.args[argc] = nullptr;
+      options_args[argc] = nullptr;
+      options.args = options_args.data();
     }
 
     // options.cwd
@@ -286,27 +287,35 @@ class ProcessWrap : public HandleWrap {
     if (!js_options->Get(context, env->env_pairs_string()).ToLocal(&env_v)) {
       return;
     }
-    if (!env_v.IsEmpty() && env_v->IsArray()) {
+    std::vector<char*> options_env;
+    std::vector<std::string> env_vals;
+    if (env_v->IsArray()) {
       Local<Array> env_opt = env_v.As<Array>();
       int envc = env_opt->Length();
       CHECK_LT(envc, INT_MAX);            // Check for overflow.
-      options.env = new char*[envc + 1];  // Heap allocated to detect errors.
+      env_vals.reserve(envc);
+      options_env.resize(envc + 1);
       for (int i = 0; i < envc; i++) {
         Local<Value> val;
         if (!env_opt->Get(context, i).ToLocal(&val)) {
           return;
         }
         node::Utf8Value pair(env->isolate(), val);
-        options.env[i] = strdup(*pair);
-        CHECK_NOT_NULL(options.env[i]);
+        env_vals.emplace_back(pair.ToString());
+        options_env[i] = const_cast<char*>(env_vals.back().c_str());
+        CHECK_NOT_NULL(options_env[i]);
       }
-      options.env[envc] = nullptr;
+      options_env[envc] = nullptr;
+      options.env = options_env.data();
     }
 
     // options.stdio
-    if (ParseStdioOptions(env, js_options, &options).IsNothing()) {
+    std::vector<uv_stdio_container_t> options_stdio;
+    if (ParseStdioOptions(env, js_options, &options_stdio).IsNothing()) {
       return;
     }
+    options.stdio = options_stdio.data();
+    options.stdio_count = options_stdio.size();
 
     // options.windowsHide
     Local<Value> hide_v;
@@ -361,18 +370,6 @@ class ProcessWrap : public HandleWrap {
       }
     }
 
-    if (options.args) {
-      for (int i = 0; options.args[i]; i++) free(options.args[i]);
-      delete [] options.args;
-    }
-
-    if (options.env) {
-      for (int i = 0; options.env[i]; i++) free(options.env[i]);
-      delete [] options.env;
-    }
-
-    delete[] options.stdio;
-
     args.GetReturnValue().Set(err);
   }
 
diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
@@ -403,7 +403,6 @@ void SyncProcessRunner::Spawn(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(result);
 }
 
-
 SyncProcessRunner::SyncProcessRunner(Environment* env)
     : max_buffer_(0),
       timeout_(0),
@@ -412,14 +411,9 @@ SyncProcessRunner::SyncProcessRunner(Environment* env)
       uv_loop_(nullptr),
 
       stdio_count_(0),
-      uv_stdio_containers_(nullptr),
       stdio_pipes_initialized_(false),
 
       uv_process_options_(),
-      file_buffer_(nullptr),
-      args_buffer_(nullptr),
-      env_buffer_(nullptr),
-      cwd_buffer_(nullptr),
 
       uv_process_(),
       killed_(false),
@@ -436,19 +430,13 @@ SyncProcessRunner::SyncProcessRunner(Environment* env)
 
       lifecycle_(kUninitialized),
 
-      env_(env) {
-}
+      env_(env) {}
 
 
 SyncProcessRunner::~SyncProcessRunner() {
   CHECK_EQ(lifecycle_, kHandlesClosed);
 
   stdio_pipes_.clear();
-  delete[] file_buffer_;
-  delete[] args_buffer_;
-  delete[] cwd_buffer_;
-  delete[] env_buffer_;
-  delete[] uv_stdio_containers_;
 }
 
 
@@ -808,12 +796,14 @@ Maybe<int> SyncProcessRunner::ParseOptions(Local<Value> js_value) {
   Local<Object> js_options = js_value.As<Object>();
 
   Local<Value> js_file;
+  const char* file_buffer;
   if (!js_options->Get(context, env()->file_string()).ToLocal(&js_file) ||
-      !CopyJsString(js_file, &file_buffer_).To(&r)) {
+      !CopyJsString(js_file, &file_buffer).To(&r)) {
     return Nothing<int>();
   }
   if (r < 0) return Just(r);
-  uv_process_options_.file = file_buffer_;
+  file_buffer_.reset(file_buffer);
+  uv_process_options_.file = file_buffer_.get();
 
   // Undocumented feature of Win32 CreateProcess API allows spawning
   // batch files directly but is potentially insecure because arguments
@@ -825,23 +815,27 @@ Maybe<int> SyncProcessRunner::ParseOptions(Local<Value> js_value) {
 #endif
 
   Local<Value> js_args;
+  char* args_buffer;
   if (!js_options->Get(context, env()->args_string()).ToLocal(&js_args) ||
-      !CopyJsStringArray(js_args, &args_buffer_).To(&r)) {
+      !CopyJsStringArray(js_args, &args_buffer).To(&r)) {
     return Nothing<int>();
   }
   if (r < 0) return Just(r);
-  uv_process_options_.args = reinterpret_cast<char**>(args_buffer_);
+  args_buffer_.reset(args_buffer);
+  uv_process_options_.args = reinterpret_cast<char**>(args_buffer_.get());
 
   Local<Value> js_cwd;
   if (!js_options->Get(context, env()->cwd_string()).ToLocal(&js_cwd)) {
     return Nothing<int>();
   }
   if (!js_cwd->IsNullOrUndefined()) {
-    if (!CopyJsString(js_cwd, &cwd_buffer_).To(&r)) {
+    const char* cwd_buffer;
+    if (!CopyJsString(js_cwd, &cwd_buffer).To(&r)) {
       return Nothing<int>();
     }
     if (r < 0) return Just(r);
-    uv_process_options_.cwd = cwd_buffer_;
+    cwd_buffer_.reset(cwd_buffer);
+    uv_process_options_.cwd = cwd_buffer_.get();
   }
 
   Local<Value> js_env_pairs;
@@ -850,12 +844,13 @@ Maybe<int> SyncProcessRunner::ParseOptions(Local<Value> js_value) {
     return Nothing<int>();
   }
   if (!js_env_pairs->IsNullOrUndefined()) {
-    if (!CopyJsStringArray(js_env_pairs, &env_buffer_).To(&r)) {
+    char* env_buffer;
+    if (!CopyJsStringArray(js_env_pairs, &env_buffer).To(&r)) {
       return Nothing<int>();
     }
     if (r < 0) return Just(r);
-
-    uv_process_options_.env = reinterpret_cast<char**>(env_buffer_);
+    env_buffer_.reset(env_buffer);
+    uv_process_options_.env = reinterpret_cast<char**>(env_buffer_.get());
   }
   Local<Value> js_uid;
   if (!js_options->Get(context, env()->uid_string()).ToLocal(&js_uid)) {
@@ -982,7 +977,7 @@ Maybe<int> SyncProcessRunner::ParseStdioOptions(Local<Value> js_value) {
   js_stdio_options = js_value.As<Array>();
 
   stdio_count_ = js_stdio_options->Length();
-  uv_stdio_containers_ = new uv_stdio_container_t[stdio_count_];
+  uv_stdio_containers_.resize(stdio_count_);
 
   stdio_pipes_.clear();
   stdio_pipes_.resize(stdio_count_);
@@ -1007,7 +1002,7 @@ Maybe<int> SyncProcessRunner::ParseStdioOptions(Local<Value> js_value) {
     }
   }
 
-  uv_process_options_.stdio = uv_stdio_containers_;
+  uv_process_options_.stdio = uv_stdio_containers_.data();
   uv_process_options_.stdio_count = stdio_count_;
 
   return Just<int>(0);
diff --git a/src/spawn_sync.h b/src/spawn_sync.h
@@ -205,15 +205,15 @@ class SyncProcessRunner {
   uv_loop_t* uv_loop_;
 
   uint32_t stdio_count_;
-  uv_stdio_container_t* uv_stdio_containers_;
+  std::vector<uv_stdio_container_t> uv_stdio_containers_;
   std::vector<std::unique_ptr<SyncProcessStdioPipe>> stdio_pipes_;
   bool stdio_pipes_initialized_;
 
   uv_process_options_t uv_process_options_;
-  const char* file_buffer_;
-  char* args_buffer_;
-  char* env_buffer_;
-  const char* cwd_buffer_;
+  std::unique_ptr<const char[]> file_buffer_;
+  std::unique_ptr<char[]> args_buffer_;
+  std::unique_ptr<char[]> env_buffer_;
+  std::unique_ptr<const char[]> cwd_buffer_;
 
   uv_process_t uv_process_;
   bool killed_;
