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

Fix Unix.create_process cmdline escaping windows #1042

Merged
merged 2 commits into from Feb 19, 2017

Conversation

Projects
None yet
4 participants
@fdopen
Contributor

fdopen commented Feb 14, 2017

A new pull request, so that #1038 (comment) isn't forgotten.

The current quoting algorithm seem to be correct, but parameters with '\t' are not quoted, although they must be quoted. However, neither '\n' nor '\011' (vertical tab) need quoting (contrary to https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/).

I've cherry-picked the test case from @nojb (slightly edited). A not included brute force test ( https://gist.github.com/anonymous/4a98fa17cffaaffc8775ab36286e3e70 ) didn't reveal any further problems (beside the ones that are imposed by the windows API).

@nojb

This comment has been minimized.

Show comment
Hide comment
@nojb

nojb Feb 14, 2017

Contributor

Thanks!

I take the liberty of piggybacking on your PR to mention that the documentation of Unix.in_channel_of_descr seems to be incorrect: it states that the function Unix.set_binary_mode_in will always fail on channels created by that function under Windows. But this is not the case (as the test shows).

Contributor

nojb commented Feb 14, 2017

Thanks!

I take the liberty of piggybacking on your PR to mention that the documentation of Unix.in_channel_of_descr seems to be incorrect: it states that the function Unix.set_binary_mode_in will always fail on channels created by that function under Windows. But this is not the case (as the test shows).

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 16, 2017

Contributor

This looks good to me. CI is unhappy because there is no Changes entry but we can fix that. I'm in favor of merging in master. @damiendoligez do you want it in 4.05 as well?

Contributor

xavierleroy commented Feb 16, 2017

This looks good to me. CI is unhappy because there is no Changes entry but we can fix that. I'm in favor of merging in master. @damiendoligez do you want it in 4.05 as well?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 16, 2017

Contributor

@nojb: yes, the doc of Unix.in_channel_of_descr is weird. What is not supported is text mode on channels over sockets.

Contributor

xavierleroy commented Feb 16, 2017

@nojb: yes, the doc of Unix.in_channel_of_descr is weird. What is not supported is text mode on channels over sockets.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Feb 16, 2017

Member

This is a bugfix that warrants a Changes entry. Let's have it both in trunk and in 4.05.

Member

damiendoligez commented Feb 16, 2017

This is a bugfix that warrants a Changes entry. Let's have it both in trunk and in 4.05.

fdopen and others added some commits Feb 14, 2017

fix win32unix cmdline quoting
'\t' must also be quoted.

@xavierleroy xavierleroy merged commit 9988846 into ocaml:trunk Feb 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

xavierleroy added a commit that referenced this pull request Feb 19, 2017

Merge pull request #1042 from fdopen/create_process_quoting
Fix Unix.create_process command-line escaping under Windows
@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 19, 2017

Contributor

Merged in trunk and cherry-picked for 4.05.

This is the first time that I cherry-pick a merge. Hope I did it right.

Contributor

xavierleroy commented Feb 19, 2017

Merged in trunk and cherry-picked for 4.05.

This is the first time that I cherry-pick a merge. Hope I did it right.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #1042 from fdopen/create_process_quoting
Fix Unix.create_process command-line escaping under Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment