From 0d0ce7a1fce7b28187ab8bd82c194908aed57158 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Feb 2020 15:52:39 +0100 Subject: [PATCH 1/4] src: fix OOB reads in process.title getter The getter passed a stack-allocated, fixed-size buffer to uv_get_process_title() but neglected to check the return value. When the total length of the command line arguments exceeds the size of the buffer, libuv returns UV_ENOBUFS and doesn't modify the contents of the buffer. The getter then proceeded to return whatever garbage was on the stack at the time of the call, quite possibly reading beyond the end of the buffer. Add a GetProcessTitle() helper that reads the process title into a dynamically allocated buffer that is resized when necessary. Fixes: https://github.com/nodejs/node/issues/31631 --- src/node_internals.h | 1 + src/node_process_object.cc | 8 ++++---- src/node_v8_platform-inl.h | 6 +++--- src/util.cc | 28 +++++++++++++++++++++++++--- test/parallel/test-process-title.js | 15 +++++++++++++++ 5 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-process-title.js diff --git a/src/node_internals.h b/src/node_internals.h index 55f5ac40ccc408..abe01e6fed897c 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -95,6 +95,7 @@ void ResetStdio(); // Safe to call more than once and from signal handlers. void SignalExit(int signal, siginfo_t* info, void* ucontext); #endif +std::string GetProcessTitle(const char* default_title); std::string GetHumanReadableProcessName(); void GetHumanReadableProcessName(char (*name)[1024]); diff --git a/src/node_process_object.cc b/src/node_process_object.cc index a1bf90c8d69fc0..7cf8c125009606 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -32,11 +32,11 @@ using v8::Value; static void ProcessTitleGetter(Local property, const PropertyCallbackInfo& info) { - char buffer[512]; - uv_get_process_title(buffer, sizeof(buffer)); + std::string title = GetProcessTitle("node"); info.GetReturnValue().Set( - String::NewFromUtf8(info.GetIsolate(), buffer, NewStringType::kNormal) - .ToLocalChecked()); + String::NewFromUtf8(info.GetIsolate(), title.data(), + NewStringType::kNormal, title.size()) + .ToLocalChecked()); } static void ProcessTitleSetter(Local property, diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index ecd8f21d1a2653..0f4a98a98551ba 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -21,12 +21,12 @@ class NodeTraceStateObserver : public v8::TracingController::TraceStateObserver { public: inline void OnTraceEnabled() override { - char name_buffer[512]; - if (uv_get_process_title(name_buffer, sizeof(name_buffer)) == 0) { + std::string title = GetProcessTitle(""); + if (!title.empty()) { // Only emit the metadata event if the title can be retrieved // successfully. Ignore it otherwise. TRACE_EVENT_METADATA1( - "__metadata", "process_name", "name", TRACE_STR_COPY(name_buffer)); + "__metadata", "process_name", "name", TRACE_STR_COPY(title.c_str())); } TRACE_EVENT_METADATA1("__metadata", "version", diff --git a/src/util.cc b/src/util.cc index 26dbfe844995eb..2a594d81ecab8e 100644 --- a/src/util.cc +++ b/src/util.cc @@ -22,6 +22,7 @@ #include "util.h" // NOLINT(build/include_inline) #include "util-inl.h" +#include "debug_utils-inl.h" #include "env-inl.h" #include "node_buffer.h" #include "node_errors.h" @@ -45,6 +46,7 @@ #include #include +#include #include #include @@ -133,10 +135,30 @@ void LowMemoryNotification() { } } +std::string GetProcessTitle(const char* default_title) { + std::string buf(16, '\0'); + + for (;;) { + const int rc = uv_get_process_title(&buf[0], buf.size()); + + if (rc == 0) + break; + + if (rc != UV_ENOBUFS) + return default_title; + + buf.resize(2 * buf.size()); + } + + // Strip excess trailing nul bytes. Using strlen() here is safe, + // uv_get_process_title() always zero-terminates the result. + buf.resize(strlen(&buf[0])); + + return buf; +} + std::string GetHumanReadableProcessName() { - char name[1024]; - GetHumanReadableProcessName(&name); - return name; + return SPrintF("%s[%d]", GetProcessTitle("Node.js"), uv_os_getpid()); } void GetHumanReadableProcessName(char (*name)[1024]) { diff --git a/test/parallel/test-process-title.js b/test/parallel/test-process-title.js new file mode 100644 index 00000000000000..dc200df6e1baf6 --- /dev/null +++ b/test/parallel/test-process-title.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../common'); +const { spawnSync } = require('child_process'); +const { strictEqual } = require('assert'); + +// FIXME add sunos support +if (common.isSunOS) + common.skip(`Unsupported platform [${process.platform}]`); +// FIXME add IBMi support +if (common.isIBMi) + common.skip('Unsupported platform IBMi'); + +const xs = 'x'.repeat(1024); +const proc = spawnSync(process.execPath, ['-p', 'process.title', xs]); +strictEqual(proc.stdout.toString().trim(), process.execPath); From 9e1203e1f604d266224fbefa9169ce1ef9ceaa94 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Feb 2020 15:52:39 +0100 Subject: [PATCH 2/4] src: remove fixed-size GetHumanReadableProcessName Remove the version of GetHumanReadableProcessName() that operates on a fixed-size buffer. The only remaining caller is Assert() which might get called in contexts where dynamically allocating memory isn't possible but as Assert() calls printf(), which also allocates memory when necessary, this commit is unlikely to make matters much worse. --- src/node_errors.cc | 5 ++--- src/node_internals.h | 1 - src/util.cc | 7 ------- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/node_errors.cc b/src/node_errors.cc index f82054c339b493..9d038e3d1683e7 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -242,12 +242,11 @@ void AppendExceptionLine(Environment* env, } [[noreturn]] void Assert(const AssertionInfo& info) { - char name[1024]; - GetHumanReadableProcessName(&name); + std::string name = GetHumanReadableProcessName(); fprintf(stderr, "%s: %s:%s%s Assertion `%s' failed.\n", - name, + name.c_str(), info.file_line, info.function, *info.function ? ":" : "", diff --git a/src/node_internals.h b/src/node_internals.h index abe01e6fed897c..64b4d489e1e184 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -97,7 +97,6 @@ void SignalExit(int signal, siginfo_t* info, void* ucontext); std::string GetProcessTitle(const char* default_title); std::string GetHumanReadableProcessName(); -void GetHumanReadableProcessName(char (*name)[1024]); void InitializeContextRuntime(v8::Local); diff --git a/src/util.cc b/src/util.cc index 2a594d81ecab8e..29bfb5d351e35b 100644 --- a/src/util.cc +++ b/src/util.cc @@ -161,13 +161,6 @@ std::string GetHumanReadableProcessName() { return SPrintF("%s[%d]", GetProcessTitle("Node.js"), uv_os_getpid()); } -void GetHumanReadableProcessName(char (*name)[1024]) { - // Leave room after title for pid, which can be up to 20 digits for 64 bit. - char title[1000] = "Node.js"; - uv_get_process_title(title, sizeof(title)); - snprintf(*name, sizeof(*name), "%s[%d]", title, uv_os_getpid()); -} - std::vector SplitString(const std::string& in, char delim) { std::vector out; if (in.empty()) From 644aae6d87294a61657760605c0795bd44a216d0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Feb 2020 21:52:33 +0100 Subject: [PATCH 3/4] fixup! work around windows quirk --- test/parallel/test-process-title.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/parallel/test-process-title.js b/test/parallel/test-process-title.js index dc200df6e1baf6..f8052704429627 100644 --- a/test/parallel/test-process-title.js +++ b/test/parallel/test-process-title.js @@ -10,6 +10,12 @@ if (common.isSunOS) if (common.isIBMi) common.skip('Unsupported platform IBMi'); +// Explicitly assigning to process.title before starting the child process +// is necessary otherwise *its* process.title is whatever the last +// SetConsoleTitle() call in our process tree set it to. +if (common.isWindows) + process.title = process.execPath; + const xs = 'x'.repeat(1024); const proc = spawnSync(process.execPath, ['-p', 'process.title', xs]); strictEqual(proc.stdout.toString().trim(), process.execPath); From 0ff7befb9002735907538d73653a341ed75b44a0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 4 Feb 2020 23:35:57 +0100 Subject: [PATCH 4/4] fixup! link to https://github.com/libuv/libuv/issues/2667 --- test/parallel/test-process-title.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/parallel/test-process-title.js b/test/parallel/test-process-title.js index f8052704429627..13a030decf887c 100644 --- a/test/parallel/test-process-title.js +++ b/test/parallel/test-process-title.js @@ -13,6 +13,7 @@ if (common.isIBMi) // Explicitly assigning to process.title before starting the child process // is necessary otherwise *its* process.title is whatever the last // SetConsoleTitle() call in our process tree set it to. +// Can be removed when https://github.com/libuv/libuv/issues/2667 is fixed. if (common.isWindows) process.title = process.execPath;