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.

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

Fixed process layer argument quoting; allows for strings with spaces. #1132

Merged
merged 1 commit into from Oct 10, 2022

Conversation

adamharrison
Copy link
Member

Noticed this slight bug; when passing in arguments with strings, we don't actually escape these even if passed in array form. Should be escaped, like with Linux.

@adamharrison adamharrison marked this pull request as draft September 30, 2022 03:47
@adamharrison adamharrison marked this pull request as ready for review September 30, 2022 03:55
@takase1121
Copy link
Member

takase1121 commented Oct 1, 2022

As I mentioned in the relevant Discord message, there are basically 3 group of behaviors - MSVCRT, CommandLineToArgvW and vendor defined. Ultimately, most programs choose to support MSVCRT because it is similar to CommandLineToArgvW (probably compatible too!). This PR probably is implementing the MSVCRT convention too.


Here's a description for that from Python:

  1. Arguments are delimited by white space, which is either a space or a tab.

  2. A string surrounded by double quotation marks is interpreted as a single argument, regardless of white space contained within. A quoted string can be embedded in an argument.

  3. A double quotation mark preceded by a backslash is interpreted as a literal double quotation mark.

  4. Backslashes are interpreted literally, unless they immediately precede a double quotation mark.

  5. If backslashes immediately precede a double quotation mark, every pair of backslashes is interpreted as a literal backslash. If the number of backslashes is odd, the last backslash escapes the next double quotation mark as described in rule 3.


Note that the definition of whitespace - \v\t are valid delimiters.

Although the MSVCRT convention allows quoting non-space arguments, this is not recommended.
While vendor-defined conventions might be incompatible with the convention, most of them do agree that non-space argument don't have be quoted. Keeping this feature would usually allow most of the simple cmd commands to run while keeping full MSVCRT compatibility. I have an example in this issue.

Other than that, a verbatim_argument option should be added.

In this mode the array will be concatenated with no escaping at all. This is the solution for node.js, but doesn't seem like it in Python. However, Python probably allows this by passing a single string rather than a list.

This option is important for aforementioned vendor-specific convention, most notably cmd. cmd simply has its own quoting convention when various flags are enabled - think /s and by providing verbatim_argument we won't interfere with possible escaping by the user.

@adamharrison
Copy link
Member Author

Sure; instead of verbatim argument; we could also simply allow for a string to be passed as well, instead of a table. Would probably be me natural.

@takase1121
Copy link
Member

takase1121 commented Oct 1, 2022

I'd agree, but if we provide a string in POSIX platforms I don't know how it would be handled. I would assume Python simply uses shlex to convert it into an argument array.

EDIT: nevermind, Python simply uses it as argv[0], we're all good.

@adamharrison
Copy link
Member Author

OK, added in the ability to specify a string; just argv[0] on POSIX; literal string on windows. Checked with python; behaviour is identical.

… And added ability to specify a literal, in the style of python.
@jgmdev
Copy link
Member

jgmdev commented Oct 10, 2022

Tested changes on windows and everything is working normally so merging

@jgmdev jgmdev merged commit dad0f79 into master Oct 10, 2022
jgmdev added a commit to lite-xl/console that referenced this pull request Oct 13, 2022
As discussed on discord the new changes to the lite-xl process api to properly
handle argument escaping on windows introduced on lite-xl/lite-xl#1132,
when passing the command as a table it double quotes all arguments and also
parenthesized statements like `cmd /c (echo hello)` which results in
`"cmd" "/c" "(echo hello)"`. This causes batch to error out as it improperly
interprets "(echo hello)" as a whole command instead of a group of statements.

To fix the issue with newer process api one would need to use a plain
string instead of a table to disable the quoting mechanism when
inappropriate.
takase1121 pushed a commit to takase1121/lite-xl that referenced this pull request Nov 14, 2022
… And added ability to specify a literal, in the style of python. (lite-xl#1132)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants