Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix OOB reads in process.title getter #31633

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 ? ":" : "",
Expand Down
2 changes: 1 addition & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ 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]);

void InitializeContextRuntime(v8::Local<v8::Context>);

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
33 changes: 24 additions & 9 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,17 +135,30 @@ void LowMemoryNotification() {
}
}

std::string GetHumanReadableProcessName() {
char name[1024];
GetHumanReadableProcessName(&name);
return name;
std::string GetProcessTitle(const char* default_title) {
std::string buf(16, '\0');

for (;;) {
const int rc = uv_get_process_title(&buf[0], buf.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking for this PR, but... in the future, might it be possible to introduce a variant of uv_get_process_title that can optionally report the size of the buffer required? something along the lines of...

  size_t actual = buf.size();
  const int rc = uv_get_process_title2(&buf[0], &actual);
  if (rc == UV_ENOBUFS) {
    buf.resize(actual);
    // then try again
  }
  // ....

Definitely minor but a bit nicer to avoid the multiple resize operations


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;
}

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::string GetHumanReadableProcessName() {
return SPrintF("%s[%d]", GetProcessTitle("Node.js"), uv_os_getpid());
}

std::vector<std::string> SplitString(const std::string& in, char delim) {
Expand Down
21 changes: 21 additions & 0 deletions test/parallel/test-process-title.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'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');

// 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;
Copy link
Member

@jasnell jasnell Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using // Flags: --title=test to explicitly set the title with a flag would work here also, right? Might be better than having a special case here for Windows

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See libuv/libuv#2667 - I believe this is a Windows-only libuv bug. Until it's fixed, it's best to call it out explicitly.


const xs = 'x'.repeat(1024);
const proc = spawnSync(process.execPath, ['-p', 'process.title', xs]);
strictEqual(proc.stdout.toString().trim(), process.execPath);
addaleax marked this conversation as resolved.
Show resolved Hide resolved