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

Process.title gets corrupted in case process.argv[2] is too long #31631

Closed
Mitko-Kerezov opened this issue Feb 4, 2020 · 3 comments
Closed
Labels
process Issues and PRs related to the process subsystem.

Comments

@Mitko-Kerezov
Copy link

Mitko-Kerezov commented Feb 4, 2020

  • Version: v10.15.1
  • Platform: Ubuntu 18.04

Please see the script below:

const printTitle = () => {
    console.log(`Process version ${process.version}`);
    console.log(`Process title ${process.title}`);
};

printTitle();
printTitle();

I am executing this script in the terminal with node script-name.js

The output is as follows:

  • With no additional command line arguments:

Process version v10.15.1
Process title node
Process version v10.15.1
Process title node

  • With a command line argument of length 700

Process version v10.15.1
Process title
Process version v10.15.1
Process title �PP��

It appears that process.title (and only process.title as far as I can tell) gets corrupted in case provess,argv[2] is of substantial length. Note that each time the second print of process.title yields a different result.

I have read the docs regarding process.title and as far as I can tell there is only an issue with assigning a large string to process.title.

@bnoordhuis
Copy link
Member

I can't reproduce exactly what you're describing but I can reproduce something:

$ ./out/Release/node -p process.title `base64 /dev/urandom | head -c 475`
./out/Release/node

$ ./out/Release/node -p process.title `base64 /dev/urandom | head -c 476`
# nothing

Note that the length of the four arguments are 512 and 513 respectively when you account for trailing nul bytes. Coincidence? I don't think so!

@bnoordhuis
Copy link
Member

Okay, found it:

char buffer[512];
uv_get_process_title(buffer, sizeof(buffer));

Node.js passes a fixed-size buffer to libuv and neglects to check the return value. Fix on the way.

@Mitko-Kerezov
Copy link
Author

@bnoordhuis
I initially stumbled upon the issue with an argument which was 700 bytes long. Bear in mind that you have to print the title at least twice in order to end up with some "corrupted" memory inside.
Having said that, interestingly the following code does not reproduce the issue:

node --eval 'console.log(process.title); console.log(process.title)' `base64 /dev/urandom | head -c 700`

and neither does the following:

node --eval 'function test() {console.log(process.title); console.log(process.title)} test()' `base64 /dev/urandom | head -c 700`

it is only with a file that I manage to reproduce it and only if the file contains a function execution.

Meaning if I have:
test.js:

console.log(process.title);
console.log(process.title);

And then execute

node test.js `base64 /dev/urandom | head -c 700`

I do not reproduce the issue.


Only with
test.js

const printTitle = () => {
    console.log(`Process title ${process.title}`);
};

printTitle();
printTitle();

and then executing:

node test.js `base64 /dev/urandom | head -c 700`

Only then do I get some memory chunks inside process.title

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Feb 4, 2020
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
@addaleax addaleax added the process Issues and PRs related to the process subsystem. label Feb 4, 2020
addaleax pushed a commit that referenced this issue Feb 7, 2020
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.

PR-URL: #31633
Fixes: #31631
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Feb 17, 2020
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

PR-URL: #31633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Feb 17, 2020
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.

PR-URL: #31633
Fixes: #31631
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 15, 2020
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

PR-URL: #31633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 15, 2020
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.

PR-URL: #31633
Fixes: #31631
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 17, 2020
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

PR-URL: #31633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 17, 2020
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.

PR-URL: #31633
Fixes: #31631
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 30, 2020
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

PR-URL: #31633
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 30, 2020
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.

PR-URL: #31633
Fixes: #31631
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants