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: #31631
nodejs · bnoordhuis · Feb 4, 2020 · Feb 4, 2020 · Feb 4, 2020 · Feb 4, 2020
commit 0d0ce7a1fce7b28187ab8bd82c194908aed57158
diff --git 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
@@ -32,11 +32,11 @@ using v8::Value;
 
 static void ProcessTitleGetter(Local<Name> property,
                                const PropertyCallbackInfo<Value>& 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<Name> property,

diff --git 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
@@ -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 <atomic>
 #include <cstdio>
+#include <cstring>
 #include <iomanip>
 #include <sstream>
 
@@ -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
@@ -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);