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.

Sign up for GitHub

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: do not modify original argv array on AIX #10633

Closed
wants to merge 1 commit into from
+12 −0
Closed

src: do not modify original argv array on AIX #10633

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions src/node_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ int main(int argc, char *argv[]) {
// calls elsewhere in the program (e.g., any logging from V8.)
setvbuf(stdout, nullptr, _IONBF, 0);
setvbuf(stderr, nullptr, _IONBF, 0);
#ifdef _AIX
Copy link
Contributor

@sam-github sam-github Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(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?

Copy link
Contributor Author

@hhellyer hhellyer Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

// AIX passes the same argv array that ps and other utilities see.
Copy link
Member

Choose a reason for hiding this comment

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

"see"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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.

// Node removes elements from argv but can't pass back updates to
// argc effectively corrupting it for other users.
char *local_argv[argc+1];
for (int i = 0; i < argc; i++) {
local_argv[i] = argv[i];
}
local_argv[argc] = nullptr;
return node::Start(argc, local_argv);
#else
return node::Start(argc, argv);
#endif // _AIX
}
#endif