Closed Bug 1321637 Opened 3 years ago Closed 3 years ago

Executing batch files with spaces in path fails

Categories

(WebExtensions :: General, defect, P1)

50 Branch
x86_64
Windows 10
defect

Tracking

(firefox52 fixed, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: erik.vikstrom, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

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
OS: Unspecified → Windows 10
Priority: -- → P1
Hardware: Unspecified → x86_64
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
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?
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.
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Spaces in path of native application manifest (Web extension) → Executing batch files with spaces in path fails
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.
Flags: needinfo?(mhowell)
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);
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.
Flags: needinfo?(mhowell)
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."
Duplicate of this bug: 1323148
Assignee: nobody → kmaglione+bmo
Whiteboard: triaged
Duplicate of this bug: 1332616
Duplicate of this bug: 1332616
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.
Attachment #8829313 - Flags: review?(mhowell) → review+
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 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 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
Attachment #8829313 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0849d4d893857b738c2701edbe5b353168a572
Bug 1321637: Fix execution of batch files with spaces in path. r=mhowell,aswan
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.
Attachment #8829313 - Flags: approval-mozilla-beta?
Attachment #8829313 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0b0849d4d893
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
evi can you verify the fix in today's nightly?  Thanks!
Flags: needinfo?(erik.vikstrom)
Tested version 54.0a1 (2017-01-25) and it works fine now!
Flags: needinfo?(erik.vikstrom)
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+
Attachment #8829313 - Flags: approval-mozilla-beta?
Attachment #8829313 - Flags: approval-mozilla-beta+
Attachment #8829313 - Flags: approval-mozilla-aurora?
Attachment #8829313 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1338327
Duplicate of this bug: 1344734
Blocks: 1338327
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.