1321637 - Executing batch files with spaces in path fails

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[evi]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce:

On Windows platform: Created a web extension using native messaging and if the "path" property in the native application manifest contains spaces -> does not work.

Moving the application to a folder without any spaces works but is not a solution.

Example:

{
   ...
    "path" : "c:\\Program Files (x86)\\Company Name\\App Name\\app.exe",
    ...
}

It doesn't work either to use relative paths as mentioned in the docs


Actual results:

Native app does not start as expected


Expected results:

Native app should have started

Comment 1

[Kris Maglione [:kmag]]

This should work, and I'm aware of existing extensions that use native messaging with applications in the Program Files directory without problems.

Can you provide more details?

Comment 2

[evi]

It's a bit hard to give more info since there aren't much logged in the console either.
Also, I am not in front of my computer right now, and won't be for a few days unfortunately.

However, I did manage to get it working still having the app in the "program files" folder by:
1. The native messaging host registry key uses windows 8.3 format to point to the manifest
2. Use a relative path in the manifest to point out the executable

But I still think the original issue should be resolved. There seems to be others having this problem too, at least that was my impression after a google search.

Comment 3

[evi]

Hi, here is some more information regarding this problem:

I created a Windows BAT script that simply prints out "Hello World" to stderr.
This program doesn't do anything at all, it is just for debugging purposes so that we get some output in the Firefox debug tool.

The program is a one-liner:

    @echo Hello World 1>&2

So, the first test was to use 8.3 format of the path in the registry (the native messaging host).
The debug tool then printed out this.

    stderr output from native app testapp: Hello World 

Result as expected, passed.

Now, change the registry key to use a path with spaces, e.g C:\Program Files (x86)\Company Name\Product Name 1.0\testapp.json

Running the test again gives this in the Firefox debug console:

    File closed  NativeMessaging.jsm:246
    stderr output from native app testapp: 'C:\Program' is not recognized as an internal or external command,
    stderr output from native app testapp: operable program or batch file.

My conclusion is that our executable never gets called

Comment 4

[Kris Maglione [:kmag]]

OK, apparently this issue is specific to invoking batch files, and is actually a Windows bug. It may be worth working around by invoking cmd.exe directly in those cases.

Comment 5

[Kris Maglione [:kmag]]

Matt, do you have any suggestions on the best way to handle this? Trying to directly launch batch files with spaces in their paths causes errors. Special casing them, and launching them directly via cmd.exe with a handful of quoting hacks should solve the problem, but it would be great if there was a better way.

Comment 6

[evi]

I just looked into the source code, and I might be totally wrong, but shouldn't the command itself also be quoted like the args are handled in the "Process.spawn" method in subprecess_worker_win.js?

https://hg.mozilla.org/mozilla-central/file/tip/toolkit/modules/subprocess/subprocess_worker_win.js#l440

E.g.

command = this.quoteString(command);

Comment 7

[Molly Howell (she/her) (mostly inactive)]

I don't think there's any way to reliably launch batch files using CreateProcess other than special casing them to use cmd as the command; in fact that's the only documented way. Might also have to insert an extra pair of quotes, because the /s switch gets the outermost pair stripped off.

Comment 8

[evi]

I just read about CreateProcess from this page:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx

There it says about the arg "lpApplicationName":

"To run a batch file, you must start the command interpreter; set lpApplicationName to cmd.exe and set lpCommandLine to the following arguments: /c plus the name of the batch file."

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1321637: Fix execution of batch files with spaces in path.

Review commit: https://reviewboard.mozilla.org/r/106420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106420/

Comment 13

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8829313 [details]
Bug 1321637: Fix execution of batch files with spaces in path.

https://reviewboard.mozilla.org/r/106420/#review107556

::: toolkit/modules/subprocess/subprocess_worker_win.js:452
(Diff revision 1)
>        args = args.map(arg => this.quoteString(arg));
>      }
>  
> +    if (/\.bat$/i.test(command)) {
> +      command = io.comspec;
> +      args = ["cmd.exe", "/s/c", `"${args.join(" ")}"`];

This function will now break if args[0] is not a copy of the command path, and I can't find that assumption documented. An addition to the Subprocess.call() documentation comments would make me feel better.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8829313 [details]
Bug 1321637: Fix execution of batch files with spaces in path.

https://reviewboard.mozilla.org/r/106420/#review107556

> This function will now break if args[0] is not a copy of the command path, and I can't find that assumption documented. An addition to the Subprocess.call() documentation comments would make me feel better.

Subprocess.jsm prepends the command path to the arguments list. This isn't something callers need to be aware of.

Comment 15

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8829313 [details]
Bug 1321637: Fix execution of batch files with spaces in path.

https://reviewboard.mozilla.org/r/106420/#review107556

> Subprocess.jsm prepends the command path to the arguments list. This isn't something callers need to be aware of.

You're right, it does; I missed that line before.

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8829313 [details]
Bug 1321637: Fix execution of batch files with spaces in path.

https://reviewboard.mozilla.org/r/106420/#review107606

r=me for the test bits, I don't know enough about the windows bits to say anything meaningful.

::: toolkit/components/extensions/test/xpcshell/head_native_messaging.js:18
(Diff revision 1)
>                                    "resource://gre/modules/Timer.jsm");
>  
>  let {Subprocess, SubprocessImpl} = Cu.import("resource://gre/modules/Subprocess.jsm", {});
>  
>  
> -let tmpDir = FileUtils.getDir("TmpD", ["NativeMessaging"]);
> +let tmpDir = FileUtils.getDir("TmpD", ["Native Messaging"]);

Add a comment referencing this bug to indicate that the space is actually important for test coverage?

::: toolkit/components/extensions/test/xpcshell/head_native_messaging.js:93
(Diff revision 1)
>        do_register_cleanup(() => {
>          registry.shutdown();
>        });
>  
>        for (let script of scripts) {
> -        let batPath = getPath(`${script.name}.bat`);
> +        let batPath = getPath(`batch ${script.name}.bat`);

same comment as above

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0849d4d893857b738c2701edbe5b353168a572
Bug 1321637: Fix execution of batch files with spaces in path. r=mhowell,aswan

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8829313 [details]
Bug 1321637: Fix execution of batch files with spaces in path.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1270359

[User impact if declined]: This prevents extensions from launching native messaging clients from batch files with spaces in their paths. This has already been reported several times, so it is likely to affect users and developers of older versions once it lands on nightly.

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Rough steps to reproduce are described in comment 0 and comment 2. This should be verified by the reporter.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Low-risk.
[Why is the change risky/not risky?]: The change is minor, and includes tests. The only existing uses this has a potential of affecting are Windows native messaging clients which point to batch files without spaces in their paths. They should, however, continue to work as expected. And given the general layout of the Windows filesystems, they rarely exist in practice.

[String changes made/needed]: None.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b0849d4d893

Comment 20

[Julien Cristau [:jcristau]]

evi can you verify the fix in today's nightly?  Thanks!

Comment 21

[evi]

Tested version 54.0a1 (2017-01-25) and it works fine now!

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 8829313 [details]
Bug 1321637: Fix execution of batch files with spaces in path.

fix for paths with spaces, verified in nightly, aurora53+, beta52+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44d3a67db0f5

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/65a200a44636