unix,doc: always copy process title when setting #1396

64 · 2017-06-30T16:17:35Z

Ensures that the user's argv is copied into a local buffer when calling uv_setup_args. Before, the argv was simply being pointed to, which meant that libuv could end up accessing invalid memory if the user decided to later edit the memory at that location. It also meant that a subsequent call to uv_set_process_title would never write more characters than the length of argv[0].

With the new changes, argv[0] is copied into a temporary buffer and any subsequent calls to uv_set_process_title will thus be able to copy as many characters as the call to uv__strdup permits. Note that on *BSD and AIX this behaviour was already in effect .

Some error checking (specifically checking the result of uv__strdup) has been added, and calls to uv__free rearranged so that in case of ENOMEM uv__free can't be called erroneously.

Finally, a warning about the thread safety of uv_get_process_title and uv_set_process_title was added to the docs.

Note: I wasn't too sure how to add a test for this, since I don't believe the current test system exposes argv.

Addresses #1395.

cjihrig · 2017-07-01T02:26:54Z

I haven't reviewed the code, but can you split this into two commits - one for the docs, and one for the code changes.

Add a small warning about uv_get_process_title / uv_set_process_title not being thread safe on platforms other than Windows. Also adds a reminder for users to call `uv_setup_args` first.

Ensures that the user's argv is copied into a local buffer when calling uv_setup_args. Before, the argv was simply being pointed to, which meant that libuv could end up accessing invalid memory if the user decided to later edit the memory at that location. It also meant that a subsequent call to uv_set_process_title would never write more characters than the length of argv[0]. With the new changes, argv[0] is copied into a temporary buffer and any subsequent calls to uv_set_process_title will thus be able to copy as many characters as the call to uv__strdup permits. Note that on *BSD and AIX this behaviour was already in effect . Some error checking (specifically checking the result of uv__strdup) has been added, and calls to uv__free rearranged so that in case of ENOMEM uv__free can't be called erroneously.

64 · 2017-07-01T04:01:10Z

I haven't reviewed the code, but can you split this into two commits - one for the docs, and one for the code changes.

Done.

saghul

LGTM

64 · 2017-07-10T21:09:22Z

bump.

saghul · 2017-07-11T06:18:44Z

@cjihrig can you PTAL too?

bnoordhuis

Thanks for the PR. LGTM modulo comment.

bnoordhuis · 2017-07-11T06:46:24Z

src/unix/proctitle.c

-  process_title.str = argv[0];
+  process_title.str = uv__strdup(argv[0]);
+  if (process_title.str == NULL)
+    return argv;


Can you hoist the common code (everything up to and including the if statement) out of the #if block?

64 · 2017-07-11T13:54:47Z

Comment addressed.

cjihrig

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/329/

bnoordhuis · 2017-07-12T10:01:19Z

https://ci.nodejs.org/job/libuv-test-commit-linux/357/nodes=armv7-wheezy/console - failures look unrelated, seems like a stray process hogging a port.

Add a small warning about uv_get_process_title() and uv_set_process_title() not being thread safe on platforms other than Windows. Also add a reminder for users to call uv_setup_args() first. Fixes: #1395 PR-URL: #1396 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Ensures that the user's argv is copied into a local buffer when calling uv_setup_args. Before, the argv was simply being pointed to, which meant that libuv could end up accessing invalid memory if the user decided to later edit the memory at that location. It also meant that a subsequent call to uv_set_process_title would never write more characters than the length of argv[0]. With the new changes, argv[0] is copied into a temporary buffer and any subsequent calls to uv_set_process_title will thus be able to copy as many characters as the call to uv__strdup permits. Note that on *BSD and AIX this behaviour was already in effect . Some error checking (specifically checking the result of uv__strdup) has been added, and calls to uv__free rearranged so that in case of ENOMEM uv__free can't be called erroneously. Fixes: #1395 PR-URL: #1396 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

cjihrig · 2017-07-12T13:47:33Z

Landed in bdc8700 and 78c1723. Thanks!

cjihrig · 2017-08-16T17:35:28Z

This causes a test failure in Node. See nodejs/node#14866.

The issue appears to be that overwriting argv[0] is no longer possible. So, the process title changes within the process, but externally, the original argv[0] is seen.

64 · 2017-08-16T17:51:28Z

I don't see argv[0] being overwritten, can you show me where? Unless I'm misunderstanding you.

cjihrig · 2017-08-16T18:20:36Z

So, the original post said:

Before, the argv was simply being pointed to, which meant that libuv could end up accessing invalid memory if the user decided to later edit the memory at that location.

and

With the new changes, argv[0] is copied into a temporary buffer and any subsequent calls to uv_set_process_title will thus be able to copy as many characters as the call to uv__strdup permits. Note that on *BSD and AIX this behaviour was already in effect .

My understanding is that with the changes in this PR, we are no longer pointing to argv. So, when uv_set_process_title() is called, we are updating a local buffer, not argv. And, when uv_get_process_title() is called, we'd be reading from that buffer. However, outside of the process, tools like ps would still report the original argv values. Is that correct? If so, that explains the test failure in the Node.js test suite.

That also leaves the question of what to do about it.

64 · 2017-08-16T18:48:34Z

You're right about the temporary buffer, but I don't believe that changing argv[0] actually changes the name of the process title. In unix/linux-core.c:uv__set_process_title the function prctl is called which I believe does the switch. Unless you're referring to a different system.

cjihrig · 2017-08-16T19:10:12Z

Changing argv[0] does change the name of the process name. I think the difference between that and prctl is explained here.

64 · 2017-08-16T19:22:25Z

My mistake then. So argv[0] needs to be changed too? We need to be careful about how much space is allocated for it, in that case. Is this only an issue on Linux?

cjihrig · 2017-08-17T15:21:41Z

Is this only an issue on Linux?

I don't think so. The Node CI lit up bright red when I updated the version of libuv. You can see the failures here: https://ci.nodejs.org/job/node-test-commit/11826/

64 · 2017-08-17T16:49:59Z

Looks like you're right, although there are some unrelated Windows failures there too.

I think just reverting the changes in unix/proctitle.c from this PR would do the trick. I'll try later and see if that works on my machine. Might also be a good idea to improve the tests in libuv to check that the title is actually being set correctly, but I'm not sure of a good method of doing that in a portable way.

cjihrig · 2017-08-17T16:56:56Z

There are some things that are significantly harder or more involved to test in C than they are from Node. A CI job or something similar to test libuv in Node before a release is cut might be nice, but are beyond the scope of this issue.

santigimeno · 2017-08-17T17:00:43Z

A CI job or something similar to test libuv in Node before a release is cut might be nice

That's indeed a very good idea. /cc @refack as I think he's now member of the nodejs build group

refack · 2017-08-17T17:00:54Z

Is this only an issue on Linux?

Just another data point: current code works on Windows (because the whole window != process architecture)

refack · 2017-08-17T17:01:31Z

it's? 😱

santigimeno · 2017-08-17T17:03:55Z

OMG sorry :(. I'll edit the comment.

refack · 2017-08-17T17:04:50Z

CI job or something similar to test libuv in Node before a release is cut might be nice, but are beyond the scope of this issue.

~~I'm not sure what would that entail?~~ Ohh like a libuv CitGM... that's a nice idea!

refack · 2017-08-17T17:05:38Z

OMG sorry :(. I'll edit the comment.

P.S. I'm almost un-offendable

cjihrig · 2017-08-17T17:07:07Z

I was thinking more like nodejs/node-v8 than CITGM. Let's move this to a separate issue though.

Ensure that argv[0] is changed when uv_set_process_title() is called. Previously, on some unix systems uv__set_process_title() was being called, but argv[0] was not modified. Partial revert of 78c1723. Refs: #1396 PR-URL: #1487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

64 added 2 commits July 1, 2017 04:46

doc: add thread safety warning for process title

50344dc

Add a small warning about uv_get_process_title / uv_set_process_title not being thread safe on platforms other than Windows. Also adds a reminder for users to call `uv_setup_args` first.

saghul approved these changes Jul 1, 2017

View reviewed changes

bnoordhuis approved these changes Jul 11, 2017

View reviewed changes

unix: hoist common code out of #if block

2fae512

cjihrig approved these changes Jul 11, 2017

View reviewed changes

cjihrig closed this Jul 12, 2017

cjihrig mentioned this pull request Aug 16, 2017

deps: upgrade libuv to 1.14.1 nodejs/node#14866

Merged

2 tasks

refack mentioned this pull request Aug 17, 2017

including downstream projects in regression tests #1486

Closed

64 mentioned this pull request Aug 17, 2017

unix: modify argv[0] when process title is set #1487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix,doc: always copy process title when setting #1396

unix,doc: always copy process title when setting #1396

64 commented Jun 30, 2017 •

edited

Loading

cjihrig commented Jul 1, 2017

64 commented Jul 1, 2017

saghul left a comment

64 commented Jul 10, 2017

saghul commented Jul 11, 2017

bnoordhuis left a comment

bnoordhuis Jul 11, 2017

64 commented Jul 11, 2017

cjihrig left a comment

bnoordhuis commented Jul 12, 2017

cjihrig commented Jul 12, 2017

cjihrig commented Aug 16, 2017

64 commented Aug 16, 2017 •

edited

Loading

cjihrig commented Aug 16, 2017

64 commented Aug 16, 2017 •

edited

Loading

cjihrig commented Aug 16, 2017

64 commented Aug 16, 2017

cjihrig commented Aug 17, 2017

64 commented Aug 17, 2017

cjihrig commented Aug 17, 2017

santigimeno commented Aug 17, 2017 •

edited

Loading

refack commented Aug 17, 2017

refack commented Aug 17, 2017

santigimeno commented Aug 17, 2017

refack commented Aug 17, 2017 •

edited

Loading

refack commented Aug 17, 2017

cjihrig commented Aug 17, 2017 •

edited

Loading

unix,doc: always copy process title when setting #1396

unix,doc: always copy process title when setting #1396

Conversation

64 commented Jun 30, 2017 • edited Loading

cjihrig commented Jul 1, 2017

64 commented Jul 1, 2017

saghul left a comment

Choose a reason for hiding this comment

64 commented Jul 10, 2017

saghul commented Jul 11, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jul 11, 2017

Choose a reason for hiding this comment

64 commented Jul 11, 2017

cjihrig left a comment

Choose a reason for hiding this comment

bnoordhuis commented Jul 12, 2017

cjihrig commented Jul 12, 2017

cjihrig commented Aug 16, 2017

64 commented Aug 16, 2017 • edited Loading

cjihrig commented Aug 16, 2017

64 commented Aug 16, 2017 • edited Loading

cjihrig commented Aug 16, 2017

64 commented Aug 16, 2017

cjihrig commented Aug 17, 2017

64 commented Aug 17, 2017

cjihrig commented Aug 17, 2017

santigimeno commented Aug 17, 2017 • edited Loading

refack commented Aug 17, 2017

refack commented Aug 17, 2017

santigimeno commented Aug 17, 2017

refack commented Aug 17, 2017 • edited Loading

refack commented Aug 17, 2017

cjihrig commented Aug 17, 2017 • edited Loading

64 commented Jun 30, 2017 •

edited

Loading

64 commented Aug 16, 2017 •

edited

Loading

64 commented Aug 16, 2017 •

edited

Loading

santigimeno commented Aug 17, 2017 •

edited

Loading

refack commented Aug 17, 2017 •

edited

Loading

cjihrig commented Aug 17, 2017 •

edited

Loading