-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[Process API] Quoting enhancements #12946
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
base: main
Are you sure you want to change the base?
[Process API] Quoting enhancements #12946
Conversation
… to SDL_PROP_PROCESS_CREATE_ARGS_POINTER on Windows
* - `SDL_PROP_PROCESS_CREATE_CMDLINE_STRING`: a string containing the program | ||
* to run and any parameters. This string is passed directly to | ||
* `CreateProcess` on Windows, and does nothing on other platforms. | ||
* This property is only important if you want to start programs that does | ||
* non-standard command-line processing, and in most cases using | ||
* `SDL_PROP_PROCESS_CREATE_ARGS_POINTER` is sufficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this option does not feel very safe to me.
What does it allow which the current behavior does not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately yes. This is an option that you shouldn't use unless you know what you're doing, but is needed because Windows have 3 main types of argument escaping (UCRT, CommandLineToArgvW, and cmd.exe) which are slightly different to each other and can cause issues. See my examples in the PR comment about cmd.exe /s /c, which is legitimately a good way to use cmd.exe and to avoid lots of escaping. If not, you'll have to escape for both cmd.exe's command line parsing AND batch processing, which eats up precious command line space (note that in Windows the command-line is 65535 characters long, and batch parsing only takes around 8000 characters, including escape).
Other language runtimes have this features as well:
- node.js(windowsVerbatimArguments):https://nodejs.org/api/child_process.html#child_processexecfilefile-args-options-callback
- Python: https://github.com/python/cpython/blob/af5799f3056b0eee61fc09587633500a3690e67e/Lib/subprocess.py#L1461
As a library, I think its better to focus towards functionality - the warning is there, so you will use that option at your own risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way could be adding a callback to allow user to customize how the arguments are escaped - like join_cmdline. That'll probably scare away users that didn't have a solid justification for using this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for context: command-line parsing is delegated to the callee application, often done as a part of libc startup code. Thus, it can't fail because of "syntax errors", so they're just propagated to the parsed argc and argv themselves.
cmd.exe inherited it's parsing from DOS, and MSVCRT and UCRT applications all have slightly different ways of parsing the command line. This means applications compiled on different compilers (e.g. MINGW64 and UCRT64) could use different schemes. The SDL escaping is good for UCRT and probably works for MSVCRT, but can fail for cmd.exe.
This PR avoids quoting strings if it does not have newlines, as well as adding
SDL_PROP_PROCESS_CREATE_CMDLINE_STRING
as an alternative.Description
This issue was described in #12945.
This PR offers two solutions:
SDL_CreateProcessWithProperties
will accept another property calledSDL_PROP_PROCESS_CREATE_CMDLINE_STRING
, which lets library users pass an escaped command line toCreateProcess
directly.This is useful for SDL wrappers that needed to allow users to call old executables that has their own command line escaping.
Take
cmd.exe /S /C
as example:/S
will strip first and last double quotes from the command line, after/C
itself.CreateProcess
{"cmd.exe", "/s", "/c", "echo \"hello world\"", NULL}
cmd.exe /s /c "echo \"hello world\""
echo \"hello world\"
cmd.exe /s /c "echo "hello world""
cmd.exe /s /c "echo "hello world""
echo "hello world"
The code should not interfere with #10857.
Existing Issue(s)
Fixes #12945.