Closed Bug 303127 Opened 19 years ago Closed 15 years ago

nsIProcess doesn't pass on the correct arguments

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: velcrospud, Unassigned)

Details

(Keywords: helpwanted) 

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5

Trying to start an antivirus program with correct arguments for it to scan a
particular file.  The program starts but it doesn't receive the correct argument
for it to start the scan.  The av program in question is Antivir
http://www.free-av.com/
The argument that it needs is:
/S*H*L="c:\testfile.txt"
(perhaps needlessly complex I know, I didn't design it)

Reproducible: Always

Steps to Reproduce:
I'm using Download Statusbar 0.9.3 extension (im the author) on firefox 1.0.6
and deerpark to test this. (includes the virus scan feature) With Antivir
installed.  I have confirmed that that string i have in the arguments array sent
to nsIProcess will work if that same string is entered manually into a windows
command prompt.
I don't know that installing the av program and this extension is necessary to
test this.  I assume there is some way to see what nsIProcess does with 
/S*H*L="c:\testfile.txt"
in it's assembleCmdLine function.
Could you provide the code you used to start the AV process, so that we may try
it and determin if it is a bug, or if it was simply used badly (or whatever
other reason there might be)?
I guess I didn't mention clearly that my code works fine for other AV programs
and their particular arguments.  It's only this AV program's argument that won't
work.  Anyway - this is the shortened version of what doesn't work: (note that
in this hard coded snippet, back slashes and doublequotes are escaped to make
the string)

var process = Components.classes["@mozilla.org/process/util;1"]
   .createInstance(Components.interfaces.nsIProcess);
var AVExecFile = Components.classes["@mozilla.org/file/local;1"]
   .createInstance(Components.interfaces.nsILocalFile);
AVExecFile.initWithPath("D:\\Program Files\\AVPersonal\\AVWIN.EXE");
var testArgs = new Array ( );
testArgs[0] = "/S*H*L=\"d:\\testfile.txt\"";
//alert(testArgs[0]);

if (AVExecFile.exists()) {
    process.init(AVExecFile);
    process.run(false, testArgs, testArgs.length);
}
I have tried this myself, and found the following:

  /S*H*L=\"d:\testfile.txt\"

That is the argument passed to the application. Those backslashes are NOT
present in the input string, and may well be the problem here.

Reporter, do other AV programs use quotes like or not?
Yes, those backslashes added to escape the quotes could be the problem - If I
use "escaped" quotes as you have listed, in a windows command prompt for Antivir
it doesn't work

This comment in nsProcessCommon claims that quotes need to be escaped
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#124
Looks like line 160 tests the character if it is a "
and then 
Line 172 adds a backslash to escape the quote

None of the other AV programs I've seen use quotes like this in their arguments,
but I haven't tested them all.  I suppose it would be good to have a second
program to test with, but this one might be different than most in that the
quotes start in the middle of an argument, instead of surrounding the complete
argument (like a filepath that has a space in it)
That comment in nsProcessCommon.cpp is (mostly) bogus. Neither \ nor " need to
be \-escaped in Windows command-lines under normal circumstances. Even if the
entire argument contains spaces, it does not need double-quoting either, so long
as the bits with spaces are quoted.

E.g. if the argument was:
  /foo="bar baz"
It would be perfectly correct as-is, no escaping or quoting is required.

As for testing what happens, I was simply launching C:\windows\system32\cmd.exe
with varying argument values, and using Process Explorer
(http://www.sysinternals.com/Utilities/ProcessExplorer.html) to see what the
actual command-line passed was.

For working around the problem until it can be fixed (confirming bug, since this
is totally bogus munging of the arguments) you can try either using single
quotes (which may or may not be supported by the application in question, but
are not standard on Windows) or simply not quoting the filename - which will
probably mean it fails for anything with spaces in :(
Assignee: darin → dougt
Status: UNCONFIRMED → NEW
Component: Networking: File → XPCOM
Ever confirmed: true
QA Contact: benc → xpcom
Could someone with a Windows build have a look here?
Flags: blocking1.9a1?
any progress here?  there are several problems.

passing an argument with a url unquoted works, in this form:
https://bugzilla.mozilla.org

passing an argument with a url unquoted does not work, in this form: https://bugzilla.mozilla.org/show_bug.cgi?id=333237
instead of one arg, two are passed as the '=' is dropped

passing an argument with a url *double* quoted does not work, in this form: "https://bugzilla.mozilla.org/show_bug.cgi?id=333237"
the argument received is: \"https://bugzilla.mozilla.org/show_bug.cgi?id=333237\", which the executible, in this case Fx, does not understand.

passing an argument with a url *single* quoted does not work, in this form: 'https://bugzilla.mozilla.org/show_bug.cgi?id=333237'
the argument received is: 
'https://bugzilla.mozilla.org/show_bug.cgi?id 333237', bizarrely the '=' is removed!

perhaps other symbols like & etc. are unhandled as well; since quoting is broken this pretty much kills using nsIProcess with arguments.
Keywords: helpwanted
Flags: blocking1.9a1? → blocking1.9-
mass reassigning to nobody.
Assignee: dougt → nobody
I'd like to point out that this bug is still ever present. After some testing and taking a look at nsProcessCommon.cpp, I realized the behavior illogical:

* If you pass an argument that contains spaces, line 149 adds unescaped double quotes around it (http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#149). See the corresponding sections at lines 153 and 202: these are indeed unescaped.
* If you pass an argument with quotes, line 166 adds escaped quotes around them.

The only logic behind this is if there are double quotes WITHIN a quoted argument.

Consider, however, the argument Devon stated:

  /S*H*L="c:\testfile.txt"

Adding a space will make line 149 turn it into an argument enclosed in double quotes, and by subsequently following lines 160-173, we have to escape the existing quotes, resulting in:

  "/S*H*L=\"c:\test file.txt\""

This is clearly not desirable.

I suggest the following changes:

* Line 149: Replace this check with a regular expression check to see if there are spaces outside of the quotes, or drop the if statement altogether.
* Line 160-173: Remove this technique of escaping quotes outright.

I'm curious how the community thinks about this.
Also, by inspecting nsProcessCommon.cpp, it is evident that only \ (backslash) and " (double quote) are escaped. No need to worry about ampersands or other characters.
This bug is still around after all. Quotes are still escaped for no reason.
I believe this bug is INVALID. We are implementing the standard way of constructing a command-line string. If you normally launch the program with /S*H*L="c:\testfile.txt" at a command prompt, the quotes are simply quoting the argument, and you can omit them when using nsIProcess.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → INVALID
The program (still talking about avscan.exe) will not function correctly when omitting the quotes using nsIProcess. It will not recognize the parameters correctly - propably because it expects the quotes to be there. However: there is just no way of passing regular, unescaped quotes to a program using nsIProcess because it will always escape them. Even if this is intended behaviour it sucks. Can't the caller of nsIProcess escape the parameters himself if he needs them escaped?!
Benjamin: this is a bug. Although you are correct to point out that the specific example (without space) works, adding spaces breaks it. To demonstrate, there is NO WAY to execute an application with the following command line arguments:

    avscan.exe /S*H*L="c:\test file.txt"

nsIProcess will convert this to

    avscan.exe "/S*H*L=\"c:\test file.txt\""
If you are typing /S*H*L="c:\test file.txt" at a command prompt, it is identical in effect to:

"/S*H*L=c:\test file.txt"

And in nsIProcess you don't need to quote it at all, because it's an array of arguments and nsIProcess will handle the shell quoting for you.
The point is not what you can or can not type in a command prompt. The called application expects the command line arguments in the following formatting:

    /S*H*L="c:\test file.txt"

It is simply not possible to execute the application with these arguments.

For clarification, executing the following DOES work:

    avscan.exe /S*H*L="c:\test file.txt"

Executing the following does NOT work:

    avscan.exe "/S*H*L=c:\test file.txt"
    avscan.exe "/S*H*L=\"c:\test file.txt\""

Also, note that nsIProcess will convert

    "/S*H*L=c:\test file.txt"

to

    "\"/S*H*L=c:\test file.txt\""

The lines of code in question are pointed out in comment 9. I maintain my suggestion of replacing line 149 with a regular expression check to see if there are spaces outside of the quotes, in which case no additional quotes are added and existing quotes are not escaped.
(In reply to comment #15)
Only if the command-line is parsed with the MSVS C/C++ runtime or equivalent
(http://msdn.microsoft.com/en-us/library/a1y7w461 and
http://msdn.microsoft.com/en-us/library/17w5ykft) or calls CommandLineToArgvW
(http://msdn.microsoft.com/en-us/library/bb776391) or does its own parsing in a
compatible way.

The command-line is a single string, trying to assign meaning to it as this
array->string conversion does is not going to work for all cases.

P.S. The *shell* quoting uses ^ as escapes and is a separate thing entirely.
I did some research which clearly shows, that entering the same statement in the command line will not only call the process with clearly different arguments but will most likely conflict with any processing done to arguments from within the process. For this I wrote a small console applcation to see which parameters are passed and what they look like.:
http://pastebin.org/86254 (perma link)

I then called the program from shell and with and without quotes from nsIProcess. I captured the output and also the "Command line"-column from Sysinternals Process Explorer. You can see for yourself, that nsIProcess will not invoke the programm correctly:
http://img695.imageshack.us/img695/9025/nsiprocess.png
Daniel, how are you splitting your arguments into the nsIProcess `args` array?

process.run(true, ["C:\\builds\\argv-test.py", "Test Argument"], 2);
python sys.argv is ['C:\\build\\argv-test.py', 'Test Argument']

process.run(true, ["C:\\builds\\argv-test.py", "/CFG=C:\\Test Argument"], 2);
python sys.argv is ['C:\\build\\argv-test.py', '/CFG=C:\\Test Argument']

James, nsIProcess is really only meant to work with the compatible parsing techniques.
I'm using Download Statusbar to invoke nsIProcess, since I do not have any Mozilla API experience or even python installed. The Download Statusbar Developer though already told how he invokes nsIProcess in post #2 https://bugzilla.mozilla.org/show_bug.cgi?id=303127#c2 

I will ask again, because it's not clear to me: why are quotes escaped at all? When calling the same prompt from the command line the quotes will not be escaped and everything works just fine.
Quotes are escaped because that is the correct way to construct a command line on Windows, given an array of arguments.
(In reply to comment #21)
> Quotes are escaped because that is the correct way to construct a command line
> on Windows, given an array of arguments.

And with that the issue is closed. Proposed solutions:
- Report bug in Avira AntiVir that their command line parsing is incorrect
- Convert your extension for Google Chrome
(In reply to comment #19)
It's a shame such (intentional?) limitations are not documented anywhere, really (I guess I get to dig out my DevMo login now). Would anyone be interested in extending the interface to not have such a limitation? Probably count it as an edge-case and not "worth it", right?
I don't think there's a bug in the core code here. Comment #2 is obviously incorrect; the correct args would be:

testArgs[0] = "/S*H*L=d:\\testfile.txt";

You can use ctypes to perform arbitrary windows API operations if you like, but there's really no point in having an XPCOM interface for something that is so arcane and windows-specific.
Bug? Probably not. Limitation? Absolutely. Anyway, I can't find a good template in DevMo, and I've run out of care for this.
You need to log in before you can comment on or make changes to this bug.