-
Notifications
You must be signed in to change notification settings - Fork 349
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
CLI::argv()
crashes on linux when /proc
is missing
#845
Comments
Well I don't think we can put that code in CLI11. That just seems like a non-standard, scary, big security hole type of thing, even if in some way it does work. Is this a fault outside of the std::runtime_error that function throws currently if it can't open the file? |
No, the error message I wouldn't call this a security hole though. It returns the same address to argv as the one passed to main, so this is not like an internal buffer in the executable which the user never has access to. It think it's more or less guaranteed that the memory will be laid out this way by ELF. Just wanted to throw an idea out there. |
Couldn't we accept argv, argc, and just avoid using them if we can do something else for unicode support? Something like |
The idea crossed my mind when first designing my patch, but I decided against it because I think this could cause unexpected behavior for users who pre-process argc and argv before parsing them. For example, Google Test will take argc/argv, extract arguments intended for test config, and leave the rest. Then a user parses the remaining arguments (on linux) OR completely intact original arguments, converted to unicode (on windows), and, I imagine, gets weird bugs somewhere because sometimes extra arguments come through and sometimes not. If we were to go the route of explicit argument conversion by the user, IMO a better idea would be to split the int main(int argc, char** argv, char** env) {
boost::nowide::args _(argc, argv, env); // Note the _ as a "don't care" name for the instance
// Use argv and env as usual, they are now UTF-8 encoded on Windows
return 0; // Memory held by args is released
} Though I am fond of CLI::argv() as a feature by itself, this could be the more fool-proof way. I volunteer to implement it if you decide it's a good idea. |
I'm not fond of using (And I don't think I understood why you needed the two step process before, but I see why now). (I hope you don't need the So the final result would look like this, I take it? int main(int argc, char** argv) {
CLI::App app{"App description"};
argv = app.set_unicode_args(argc, argv);
CLI11_PARSE(app, argc, argv);
return 0;
} CLI::App could provide the storage for the new arg string that way. You could even allow |
The previous code snippet with Something like this (some name bikeshedding is in order): int main(int argc, char** argv) {
CLI::App app{"App description"};
argv = app.ensure_utf8(argv);
CLI11_PARSE(app);
return 0;
} I'll ping you when a provisional PR is ready. Also, do you wish to get rid of the |
Let's just stick with the explicit API. |
(though |
There's nothing I can do with it besides throw an exception when it does not match the value I get from CommandLineToArgvW. And at that point we are sanity-checking
I'm not sure about this one. Implement this and you start having to answer difficult design questions:
In any case, the PR (#923) is already mostly done, I suggest we move to discuss details there. |
We want a consistent interface between Windows and non-windows. On non-windows you'd need the |
Fixes #845 as discussed. Comparing the two approaches of getting `argv`: 1. The "old" way, through `CLI::argv()`: ✔️ Works automatically and almost everywhere ❌ Small abstraction overhead on macOS ❌ Does not work in weird edge-cases such as missing `/proc` 2. This PR, through `app.ensure_utf8`: ✔️ True zero-overhead abstraction: you don't pay for what you don't use ✔️ Less moving parts than the "old" approach, probably can't be broken ❌ Requires extra code to be written by the user (which is sad because ideally everyone should use this by default) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
When
/proc
is missing on a Linux system, callingCLI::argv()
fails withcould not open /proc/self/cmdline for reading
.I was aware of this while implementing Unicode, but thought that this is an extraordinary scenario of a broken Linux filesystem. However apparently this is a common occurrence when a user calls
chroot
before running the application.There is another way of obtaining argc and argv, but I'm a little scared of it. It does seem to work though.
The idea here is to obtain a stack address nearby from
__environ
, then walk back, counting steps. When dereferencing a pointer as-if an int* reveals that it is equal to the amount of steps, it means we have reached a pointer to argc and walked through the entire array of argv.One problem is that this has to be done before
main
, otherwise user might callsetenv
and change the pointer to environ (I presume). And if this is prepared in advance we might as well discard the/proc/self/cmdline
approach altogether. At least no files are read so the setup would be pretty fast.The text was updated successfully, but these errors were encountered: