OCaml Forge

Detail: [#1170] Oasis, when running commands, doesn't properly escape paths

Bugs: Browse | Download .csv | Monitor

[#1170] Oasis, when running commands, doesn't properly escape paths

Date:
2012-06-12 08:21
Priority:
3
State:
Open
Submitted by:
Jonathan Protzenko (mozjonathan)
Assigned to:
Sylvain Le Gall (gildor-admin)
Product:
None
Due in version:
None
Resolution:
None
Severity:
None
Version:
None
Component:
None
Operating System:
Windows XP
 
URL:
Summary:
Oasis, when running commands, doesn't properly escape paths

Detailed description
setup.ml doesn't work when the path to ocaml contains spaces (default configuration for the windows installer). I think the path to the executables should be run through Filename.quote, with the caveat that in a cygwin development environment using a native ocaml build, Sys.os_type is Win32 so Filename.quote will perform a windows-quote, whereas the expected quote is a unix-quote (I think).

Followup

Message
Date: 2012-07-12 19:10
Sender: Edgar Friendly

Digging further into the rabbit hole, I've found the following sources that seem to do a good job at elucidating filename quoting for system():
[1] http://msdn.microsoft.com/en-us/library/17w5ykft%28v=vs.85%29.aspx
[2] http://superuser.com/a/238813

The first clarifies whitespace as " \t" and gives specific rules for how '\"' and '\\' (double-quote and backslash) are handled:
* backslashes not before a double-quote are literal
* an even number of backslashes before a double quote means half that many backslashes and a string-delimiter-quote (starts or ends quotation)
* an odd number of backslashes before a double quote means half that many backslashes and a literal double-quote character

The second gives a list of special characters that need quoting to avoid being interpreted specially: &<>()@^|
Apparently there's also a special case that if none of these characters are present and there's a quoted executable name with spaces in between then the quotes stay.

From what I can tell, the "" trick is really just putting an extra layer of quotes around the whole thing so that "old behavior" kicks in and you get the expected processing from the first reference without the special-case quote-preserving magic described in source #2. In conclusion, I think for correctness, it's necessary to trigger Filename.quote on all arguments containing " \t\n\"". I'm not sure about the extra double quotes around the whole thing - this page[3] indicates that this may fail if there's no space anywhere.

[3] http://me.abelcheung.org/2009/01/19/windows-process-execution-on-php-with-double-quotes/
Date: 2012-07-12 18:07
Sender: Sylvain Le Gall

Seem like the quoting itself provided by Filename.quote is not a problem, at least for OASIS (there are corner case of Filename.quote but to be honest, I think it already cover 99% of all path that we will have in Windows).

The only refinement I need, was to know which character triggers the need of a Filename.quote. It seems that it's " \t\n\"" (can you confirm thelema?)

The other issue was about how to quote the program name (<> argument), because the quoting in this case is different, i.e. you need "" rather than " (don't ask this a kind of standard bug of windows).

If the set of char that trigger the quote is ok, I can integrate it and close the bug.

The version provided by oasis 0.3 already trigger for the most common problem which are space in the filename.

FYI, it has been tested and works (in fact there is 50% chance that you also test it when you run on Windows, because the flexlink executable is probably located in "Program Files").
Date: 2012-07-12 17:55
Sender: Edgar Friendly

Maybe I should file a bug against the builtin quoting routine - it returns the nonsensical ""\\\ when given \\\ as input. (the correct quote should be "\\\\\\")
Date: 2012-07-12 16:48
Sender: Jonathan Protzenko

I think there's already a valid quoting routine for Windows inside the Filename module. Why don't we expose both quoting styles in Filename? We could expose [Filename.quote_unix] and [Filename.quote_win32] besides the platform-native [Filename.quote]. I think I could make a point with Damien tomorrow and see if he's willing to do that for the next 4.00 release/
Date: 2012-07-12 16:05
Sender: Edgar Friendly

I ported the C++ code here[1] and got the following ocaml function to do argument escaping under windows:
[1] http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx

let escape_arg_win arg =
(* TODO: implement String.contains_any to improve performance by not escaping arguments that don't need it
if not (String.contains_any " \t\n\"" arg) then
arg
else *)
let b = Buffer.create (String.length arg) in
Buffer.add_char b '"';
let i = ref 0 in
let backslash_count = ref 0 in
while !i < String.length arg do
backslash_count := 0;
while !i < String.length arg && arg.[!i] = '\\' do
incr i; incr backslash_count;
done;
if !i >= String.length arg then
Buffer.add_string b (String.make (2 * !backslash_count) '\\')
else if arg.[!i] = '"' then (
Buffer.add_string b (String.make (2 * !backslash_count + 1) '\\');
Buffer.add_char b '"';
incr i;
) else (
Buffer.add_string b (String.make !backslash_count '\\');
Buffer.add_char b arg.[!i];
incr i;
)
done;
Buffer.add_char b '"';
Buffer.contents b
Date: 2012-07-06 08:32
Sender: Sylvain Le Gall

The fix is even worse:
http://darcs.ocamlcore.org/cgi-bin/darcsweb.cgi?r=oasis/oasis;a=headblob;f=/src/oasis/OASISExec.ml

32 if Sys.os_type = "Win32" then
33 if String.contains cmd ' ' then
34 (* Double the 1st double quote... win32... sigh *)
35 "\""^(Filename.quote cmd)
36 else
37 cmd
38 else
39 Filename.quote cmd

I still need to refine it... So I keep this bug open. However, you should have no problem running with command that has ' ' in it with OASIS 0.3.
Date: 2012-06-12 11:37
Sender: ygrek

ftr https://github.com/protz/ocaml-installer/issues/14
Date: 2012-06-12 08:27
Sender: Jonathan Protzenko

Here's a quick-and-dirty way to properly quote in the setup.ml context:

let quote c =
if Sys.command "cygpath --version" == 0 then
(* We should probably be using [Filename.unix_quote] except that function
* isn't exported. Users on Windows will have to live with not being able to
* install OCaml into c:\o'caml. Too bad. *)
Printf.sprintf "'%s'" s
else
Filename.quote s

Attached Files:

Changes:

Field Old Value Date By
assigned_tonone2012-07-06 08:33gildor-admin