src: do not modify original argv array on AIX

[hhellyer]

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: #10607

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

src

[hhellyer]

@gibfahn - Patch for AIX command line issue.

[hhellyer]

I did consider making sure argv was constant and never directly modified on all platforms but the changes would have been more invasive. There is an argument for doing that, partly as good behaviour but also because at the end of Init in node.cc it does look like the original argv is replaced with the modified one. Until I checked further I assumed that was going to modify the process arguments displayed by ps and other tools.

(A quick look on stack overflow and a test debunked that - you need to overwrite the contents original argument strings not the pointers to them in argv.)

[sam-github]

(EDIT: was nulling) duplicating the argv pointers is valid for all systems, maybe better to remove the AIX conditionality to ensure the code is compiled and run for all systems, making it easier to maintain?

[hhellyer]

I agree but there's some weirdness about how these are handled which stopped me doing it in the first place. AIX is the only one which actually uses the argv it gives you elsewhere (as far as I can tell). On Linux/Mac if you want to change the command line arguments you need to change the original strings, which are still referred to by the duplicated array so it doesn't block that at all. Also most of the initialisation code refers to the array passed in as argv which makes it a little confusing if it's not actually the original argv.

I'm happy for it to be there on all platforms though I'm also a little concerned that would look like a failed attempt to duplicate the strings in argv as I'm only copying the pointers not the arguments.

(There's also case that it would be safest to copy the lot - possibly in node::Start as that might be better when node is embedded but I don't know if that's a real use case.)

[richardlau]

"see"?

[hhellyer]

"use" might be better - on the other had it does make it clear this isn't private data. It's visible from outside of the node process.

[gibfahn]

CI: https://ci.nodejs.org/job/node-test-commit/7044/

[richardlau]

@sxa555 When Node.js is built as a shared library does Node::Start get called? If so, is the in place modification of the passed in argv likely to be an issue?

[richardlau]

I'm wondering if the right place to fix this is upstream in libuv's uv_setup_args (called by Start(int argc, char** argv) in node.cc).

e.g. deps/uv/src/unix/aix.c:

char** uv_setup_args(int argc, char** argv) {
  return argv;
}

compare to deps/uv/src/unix/proctitle.c, which returns a copy.

[mhdawson]

I think I'd lean towards Richard's suggestion. If we can keep the platform specific code in libuv thats good and it also solves the same potential use for other libuv consumers.

[hhellyer]

I'm not sure about the libuv question. On the one hand libuv then owns the original argv, always, on the other it means it always has to do a deep copy on every platform. (I'm working it through - not sure I've reached a conclusion yet.)

In the meantime I'm now worried about the use of node::Start and uv_setup_args. On the platforms where changing the process title is supported libuv assumes it's received the original process argv and that the strings contained in the elements of argv occupy contiguous memory.

If the argv passed to node::Start contains strings that were individually allocated then will uv_set_process_title potentially overwrite memory after the first string? (Edit - I think it should assert but passing uv_setup_args something that's not the real argv will confuse uv_set_process_title.) (If that's right then should I move uv_setup_args into main() so that it's not called by when node is built as a shared library?)

I'll do some more investigation. :-)

[richardlau]

On the platforms where changing the process title is supported

Another reason to change libuv is that we could also add support for changing the process title on AIX based on the observations in #10607.

[hhellyer]

We certainly could enhance libuv so it can change the process title on AIX that still leaves the question open of whether libuv should always perform a deep copy of the argument list. (Other than Linux and Mac it doesn't look like platforms like BSD or Solaris compile proctitle.c instead of the default implementation of uv_setup_args. I don't want to leave a the problem unfixed for those or new platforms.)

One option is to ensure argv is always copied - check if libuv returns the original and copy if it did. It does feel a little messy but the argv array is an odd chunk of memory, it's accessible to things other than the current process. That removes the issue that Node modifies the argv it was passed but has no way to update tell the caller that argc changed. I'm definitely open to alternative suggestions - I'd like to make sure this isn't a latent issue on other platforms as well though.

I can look at enabling changing the process title on AIX as a separate issue.

[hhellyer]

I've raised libuv/libuv#1187 to enable the changing the process title on AIX.

[hhellyer]

libuv/libuv#1187 has been approved, I will cancel this PR.