diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 2853b0dc129fc3..d27ca7da7b587b 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -116,9 +116,10 @@ class ProcessWrap : public HandleWrap { return Just(stream); } - static Maybe ParseStdioOptions(Environment* env, - Local js_options, - uv_process_options_t* options) { + static Maybe ParseStdioOptions( + Environment* env, + Local js_options, + std::vector* options_stdio) { Local context = env->context(); Local stdio_key = env->stdio_string(); Local stdios_val; @@ -132,8 +133,7 @@ class ProcessWrap : public HandleWrap { Local stdios = stdios_val.As(); 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 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( + (*options_stdio)[i].flags = static_cast( 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(); } } else if (type->StrictEquals(env->overlapped_string())) { - options->stdio[i].flags = static_cast( - 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_CREATE_PIPE | UV_READABLE_PIPE | + UV_WRITABLE_PIPE | UV_OVERLAPPED_PIPE); + if (!StreamForWrap(env, stdio).To(&(*options_stdio)[i].data.stream)) { return Nothing(); } } 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(); } } else { @@ -174,8 +174,8 @@ class ProcessWrap : public HandleWrap { } CHECK(fd_value->IsNumber()); int fd = FromV8Value(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 file_v; if (!js_options->Get(context, env->file_string()).ToLocal(&file_v)) { @@ -251,23 +249,28 @@ 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 options_args; + std::vector args_vals; + if (argv_v->IsArray()) { Local js_argv = argv_v.As(); 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); for (int i = 0; i < argc; i++) { Local 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.resize(args_vals.size() + 1); + for (size_t i = 0; i < args_vals.size(); i++) { + options_args[i] = const_cast(args_vals[i].c_str()); + CHECK_NOT_NULL(options_args[i]); } - options.args[argc] = nullptr; + options_args.back() = nullptr; + options.args = options_args.data(); } // options.cwd @@ -286,27 +289,37 @@ 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 options_env; + std::vector env_vals; + if (env_v->IsArray()) { Local env_opt = env_v.As(); 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); for (int i = 0; i < envc; i++) { Local 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[envc] = nullptr; + options_env.resize(env_vals.size() + 1); + for (size_t i = 0; i < env_vals.size(); i++) { + options_env[i] = const_cast(env_vals[i].c_str()); + CHECK_NOT_NULL(options_env[i]); + } + options_env.back() = nullptr; + options.env = options_env.data(); } // options.stdio - if (ParseStdioOptions(env, js_options, &options).IsNothing()) { + std::vector 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 hide_v; @@ -361,18 +374,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 index 574afe60df8efe..47c4aaef348433 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -403,7 +403,6 @@ void SyncProcessRunner::Spawn(const FunctionCallbackInfo& 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,12 @@ 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 +795,14 @@ Maybe SyncProcessRunner::ParseOptions(Local js_value) { Local js_options = js_value.As(); Local 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(); } 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 +814,27 @@ Maybe SyncProcessRunner::ParseOptions(Local js_value) { #endif Local 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(); } if (r < 0) return Just(r); - uv_process_options_.args = reinterpret_cast(args_buffer_); + args_buffer_.reset(args_buffer); + uv_process_options_.args = reinterpret_cast(args_buffer_.get()); Local js_cwd; if (!js_options->Get(context, env()->cwd_string()).ToLocal(&js_cwd)) { return Nothing(); } 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(); } 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 js_env_pairs; @@ -850,12 +843,13 @@ Maybe SyncProcessRunner::ParseOptions(Local js_value) { return Nothing(); } 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(); } if (r < 0) return Just(r); - - uv_process_options_.env = reinterpret_cast(env_buffer_); + env_buffer_.reset(env_buffer); + uv_process_options_.env = reinterpret_cast(env_buffer_.get()); } Local js_uid; if (!js_options->Get(context, env()->uid_string()).ToLocal(&js_uid)) { @@ -982,7 +976,7 @@ Maybe SyncProcessRunner::ParseStdioOptions(Local js_value) { js_stdio_options = js_value.As(); 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 +1001,7 @@ Maybe SyncProcessRunner::ParseStdioOptions(Local 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(0); diff --git a/src/spawn_sync.h b/src/spawn_sync.h index 4478487c8f403e..9c8b0c563c4c45 100644 --- 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_containers_; std::vector> 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 file_buffer_; + std::unique_ptr args_buffer_; + std::unique_ptr env_buffer_; + std::unique_ptr cwd_buffer_; uv_process_t uv_process_; bool killed_;