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

Incorrect ps output for node process on AIX. #10607

Closed
hhellyer opened this issue Jan 4, 2017 · 7 comments
Closed

Incorrect ps output for node process on AIX. #10607

hhellyer opened this issue Jan 4, 2017 · 7 comments
Labels
aix Issues and PRs related to the AIX platform. question Issues that look for answers.

Comments

@hhellyer
Copy link
Contributor

hhellyer commented Jan 4, 2017

  • Version: v6.9.3
  • Platform: AIX dal-pacha 1 6 00F460A94C00
  • Subsystem:

The command line arguments to the node process seem to be becoming corrupted on AIX when displayed by the ps command. For example if I run a simple script and use a command line option:

> node --abort_on_uncaught_exception --experimental_extras --max_old_space_size=300 --no-warnings simplehttp.js opt1 opt2 opt3

The output of running the ps command is:

[hhellyer@dhcp-9-20-187-94 nodereport.git]$ ps -f 3654
  UID   PID  PPID   C STIME   TTY           TIME CMD
  hhellyer 16187482  7274616   1 10:18:00  pts/0  0:00 node simplehttp.js opt1 opt2 opt3 simplehttp.js opt1 opt2 opt3

It looks like the script name and arguments have moved up but not cleared from their original position. It doesn’t seem to be specific to using v8 arguments beginning with --, using -r to require a module causes the same problem.

If I run the same script with the same options on Linux or Mac the output from ps is correct. e.g from Linux:

UID        PID  PPID  C STIME TTY          TIME CMD
hhellyer 10015  2888  0 15:19 pts/9    00:00:00 node --abort_on_uncaught_exception --experimental_extras --max_old_space_size=300 --no-warnings simplehttp.js opt1 opt2 opt3

which is exactly what was on the command line. On AIX if I run a script that prints process.argv and process.execArgv with the same options they are in in the correct positions, it's only the output from ps that looks wrong. It may be something specific in how the options are handled on AIX or it may be node always corrupts argv but that AIX is the only platform that gives the process the real argv array that’s also used by the ps command.

@nodejs/platform-aix - Probably worth the AIX team looking at this in the first instance however it may be a more general problem.

@gibfahn gibfahn added the aix Issues and PRs related to the AIX platform. label Jan 4, 2017
@mscdex mscdex added the question Issues that look for answers. label Jan 4, 2017
@jBarz
Copy link
Contributor

jBarz commented Jan 5, 2017

Node modifies the argv array in src/node.cc stripping off all the options leaving only the script file and parameters.
The "ps" utility regenerates the command line options from the modified argv but using the same original argc value. It seems like on other platforms, the "ps" utility does not regenerate from argv but from a cache where the original options were stored.

@hhellyer
Copy link
Contributor Author

hhellyer commented Jan 5, 2017

After a quick google and a test it looks like caching isn’t quite what’s happening.
It looks like on Linux and Mac the command line arguments are in a memory, starting at the address of the first string (argv[0]) and separated by nulls. If you want to change the command line ps shows then on Mac and Linux you need to actually overwrite the contents of the original strings. Changing the pointers in the argv array won’t do it - the strings themselves need to change.

Assuming Node doesn’t intend to hide v8 arguments from ps then this bug only affects AIX. (If Node did intend to change the ps output then I suspect that fixing that now might break a surprising number of things that read the ps output - monitoring scripts and the like.) Either way it should probably just be fixed for AIX so that AIX is consistent with the behaviour on other platforms and doesn’t display junk.

If I can sort my AIX build environment out I’ll have a look at copying argv to a new array the startup code can safely modify but it should be a fairly trivial fix.

hhellyer added a commit to hhellyer/node that referenced this issue Jan 5, 2017
AIX passes the same argv array that ps and other utilities see to
main(). Node removes elements from argv but as it can't pass back
updates to argc which is passed by value to main() it effectively
corrupts that array for other users.

Fixes: nodejs#10607
@hhellyer
Copy link
Contributor Author

I've prototyped the change to libuv on a branch in my node repository, now I need to clone libuv, test the change there on AIX and raise the pull request against libuv. If anyone is interested the change is here:
https://github.com/hhellyer/node/tree/uv_cmdline_aix

@hhellyer
Copy link
Contributor Author

The pull request for libuv libuv/libuv#1187 has been approved. I will close the PR for duplicating argv on AIX (#10633).

I think I need to raise another PR to enable the test for setting process.title (test/simple/test-setproctitle.js) on AIX once that change has been committed into libuv and Node has taken that level of libuv. It's a very small change but is there a way to tag that so it doesn't get lost?

richardlau added a commit to richardlau/nodereport that referenced this issue Jan 20, 2017
Node.js currently rewrites the process command line on AIX.
Refs: nodejs/node#10607
rnchamberlain pushed a commit to nodejs/node-report that referenced this issue Jan 25, 2017
Node.js currently rewrites the process command line on AIX.
Refs: nodejs/node#10607

PR-URL: #41
Reviewed-By: Richard Chamberlain <richard_chamberlain@uk.ibm.com>
Reviewed-By: Howard Hellyer <hhellyer@uk.ibm.com>
@cjihrig cjihrig closed this as completed in 8514269 Feb 9, 2017
@gibfahn
Copy link
Member

gibfahn commented Feb 9, 2017

I think I need to raise another PR to enable the test for setting process.title (test/simple/test-setproctitle.js) on AIX once that change has been committed into libuv and Node has taken that level of libuv. It's a very small change but is there a way to tag that so it doesn't get lost?

@hhellyer Time to raise this PR?

italoacasas pushed a commit that referenced this issue Feb 13, 2017
Fixes: #10165
Fixes: #9856
Fixes: #10607
Fixes: #11104
PR-URL: #11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@hhellyer
Copy link
Contributor Author

@gibfahn - Yes, I'll change it to match the libuv test which now only skips the test for SunOS. The libuv update also enabled setting the process title on MVS and now the libuv runs for everything except SunOS. (I just need to try the test out in those two environments first.)

italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 14, 2017
Fixes: nodejs#10165
Fixes: nodejs#9856
Fixes: nodejs#10607
Fixes: nodejs#11104
PR-URL: nodejs#11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@hhellyer
Copy link
Contributor Author

Change for enabling the test in Node.js raised as PR #11416

krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
Fixes: nodejs#10165
Fixes: nodejs#9856
Fixes: nodejs#10607
Fixes: nodejs#11104
PR-URL: nodejs#11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Fixes: #10165
Fixes: #9856
Fixes: #10607
Fixes: #11104
PR-URL: #11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Fixes: #10165
Fixes: #9856
Fixes: #10607
Fixes: #11104
PR-URL: #11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Fixes: #10165
Fixes: #9856
Fixes: #10607
Fixes: #11104
PR-URL: #11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Fixes: nodejs/node#10165
Fixes: nodejs/node#9856
Fixes: nodejs/node#10607
Fixes: nodejs/node#11104
PR-URL: nodejs/node#11094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aix Issues and PRs related to the AIX platform. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

4 participants