-
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
Open
takase1121
wants to merge
6
commits into
libsdl-org:main
Choose a base branch
from
takase1121:process/quoting-enhancements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
91bf90a
Do not quote arguments if no whitespaces are inside it
takase1121 5a18d93
Disable newline conversion in childprocess
takase1121 ec3ffad
Test for newlines in process arguments
takase1121 8ce8ef1
Add support for SDL_PROP_PROCESS_CREATE_CMDLINE_STRING as alternative…
takase1121 7d0aac0
Fix const issues
takase1121 88211e1
Use SDL_strpbrk instead of strpbrk
takase1121 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.