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

Lack of quoting helpers for use with Unix.open_process* #6107

Closed
vicuna opened this issue Jul 30, 2013 · 8 comments
Closed

Lack of quoting helpers for use with Unix.open_process* #6107

vicuna opened this issue Jul 30, 2013 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Jul 30, 2013

Original bug ID: 6107
Reporter: Camarade_Tux
Status: acknowledged (set by @xavierleroy on 2013-08-01T08:59:35Z)
Resolution: open
Priority: normal
Severity: feature
Category: platform support (windows, cross-compilation, etc)
Related to: #4981 #5138 #6096 #7672
Monitored by: @alainfrisch

Bug description

The open_process family of functions runs the given string through the shell. On Windows it uses cmd.exe (by default). Unfortunately, quoting for cmd.exe is difficult.

This page explains a bit how to quote arguments properly: http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx .

I believe there should be helpers to quote for cmd.exe: the Unix module provides functions that are implemented but unusable on Windows. Currently, the known victims include ocamlbuild and ocamlfind, both of which will obviously have troubles relying on a separate library like Batteries or Core. I expect there are not the only ones.

The documentation should also be updated in order to explain that each argument should be quoted and that functions to do so are available.

@vicuna
Copy link
Author

vicuna commented Jul 30, 2013

Comment author: @lefessan

Have you tried Filename.quote ?

@vicuna
Copy link
Author

vicuna commented Jul 30, 2013

Comment author: Camarade_Tux

It's insufficient for cmd.exe.

Quoting http://msdn.microsoft.com/en-us/library/bb490954.aspx :
If you use the special characters <, >, |, & or ^, you must precede them with the escape character (^) or quotation marks.

Filename.quote doesn't pay attention to these characters and shouldn't. What would be needed is another version of quote, specifically made for cmd.exe.

(and btw, testing that a round-trip doesn't alter the arguments should be part of the testsuite)

@vicuna
Copy link
Author

vicuna commented Jul 30, 2013

Comment author: Camarade_Tux

Also, I believe that having functions of the create_process* family be able to return values of type {in,out}_channel like the functions of the family open_process* could solve the issue for almost all codes.

@vicuna
Copy link
Author

vicuna commented Aug 1, 2013

Comment author: @xavierleroy

Thanks for the link. I agree it would make sense to quote command lines passed to cmd.exe (Sys.command, Unix.create_process*) on top of the normal quoting of arguments that every user of these functions should do using Filename.quote.

Since the issue is rather tricky and Damien Doligez already spent a lot of time figuring out Filename.quote under Windows, sample code and tests are most welcome.

@vicuna
Copy link
Author

vicuna commented May 14, 2014

Comment author: @alainfrisch

Changing Severity, since this is not really a bug.

@vicuna
Copy link
Author

vicuna commented Nov 26, 2017

Comment author: @xavierleroy

Pull request here: #1492

@xavierleroy
Copy link
Contributor

Still trying to get #1492 reviewed and this issue closed.

@damiendoligez
Copy link
Member

Fixed by #1492

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants