Skip to content

Commit

Permalink
src: fix OOB reads in process.title getter
Browse files Browse the repository at this point in the history
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: nodejs#31631
  • Loading branch information
bnoordhuis committed Feb 4, 2020
1 parent 60de60a commit 0d0ce7a
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 10 deletions.
1 change: 1 addition & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -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]);

Expand Down
8 changes: 4 additions & 4 deletions src/node_process_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions src/node_v8_platform-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
28 changes: 25 additions & 3 deletions src/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -45,6 +46,7 @@

#include <atomic>
#include <cstdio>
#include <cstring>
#include <iomanip>
#include <sstream>

Expand Down Expand Up @@ -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]) {
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-process-title.js
Original file line number Diff line number Diff line change
@@ -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);

0 comments on commit 0d0ce7a

Please sign in to comment.