157347 - Improper parsing of spaces on behalf of helper apps

Status: VERIFIED FIXED

(NSPR :: NSPR, defect)

Description

[Gili Tzabari]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.1a+) Gecko/20020703
BuildID:    2002070308

Sample batch file:

rem **play.cmd**
d:\mp3\z\z -r -f "%1" %2 %3 %4 %5 %6 %7 %8 %9


Reproducible: Always
Steps to Reproduce:
1. Associate *.mp3 with the above batch file
2. Hit the about URL
3. Notice how the space in the filename causes half the MP3 to be passed as %1
and the other into %2.
4. I expect %1 to contain the *entire* filename, including spaces

Comment 1

[Mike Kaply [:mkaply]]

I'm going to confirm this because we do have a bug, but your CMD file 
is broke as well.

You should not be putting double quotes around the first parameter 
(%1)

If a paremeter ends up quoted like this:

""a b c""

which it will after I have finished my fix, it won't work.

I should have a fix for this tomorrow.

Comment 2

[Mike Kaply [:mkaply]]

OK, I'm moving this to NSPR. I'll attach an explanation when I attach the patch.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Beginning of fix

I don't know what I was thinking when I left this code in NSPR.

The idea that you could ever look at parameters that were passed to you and
somehow programmatically decide whether quotation marks should be escaped is
ludicrous.

The code should simply append the parameters together and give them to the OS
API.

If parameters were supposed to be in quotes (like a filename with spaces), the
API that called us should have put them in quotes.

And if it did, we shouldn't escape those quotes.

So I am totally cleaning this up.

Note that I have to fix some stuff in helper app to go with this, and while
fixing this now won't break anything any more than it is broken now, I can't
fix helper app until this fix is in.

Thanks

Comment 4

[Wan-Teh Chang]

Comment on attachment 91381 [details] [diff] [review]
Beginning of fix

>+        strcat(cmdLine, *arg);
>+        strcat(cmdLine, " ");
>+    }
>+    cmdLine[strlen(cmdLine-1)]  = '\0';
>     return 0;

There are two problems.

1. The cmdLine buffer is not null-terminated initially.
This will cause problem with the first strcat call.

2. strlen(cmdLine-1) should be strlen(cmdLine)-1.
Actually, if you null-terminate the cmdLine buffer
before going into the for loop, you can delete the
"cmdLine[strlen(cmdLine)-1] = '\0';" statement
because the strcat calls will null-terminate the
cmdLine buffer.

Comment 5

[Mike Kaply [:mkaply]]

I was actually trying to replace the last space with the null termination.

I'll attach a new patch.

Comment 6

[Mike Kaply [:mkaply]]

Attachment: update

ok, next try :)

Comment 7

[Wan-Teh Chang]

Comment on attachment 91402 [details] [diff] [review]
update

Did you compile this code? :-)

You removed the local variable 'p', but it is still
being used.

>     p = *cmdLine = PR_MALLOC(cmdLineSize);
>     if (p == NULL) {
>         return -1;
>     }

This should just say:

      *cmdLine = PR_MALLOC(cmdLineSize);

>     for (arg = argv+1; *arg; arg++) {
>         /* Add a space to separates the arguments */
>         if (arg > argv + 1) {
>             *p++ = ' '; 
>         }

The if statement and the comment should be deleted.

>+        strcat(cmdLine, *arg);
>+        strcat(cmdLine, " ");
>+    }
>+    cmdLine[strlen(cmdLine)-1]  = '\0';

I just found that the very last strcat call will write
a null character beyond the end of the cmdLine buffer.
I suggest you replace this block of code by

	  /* Add a space to separates the arguments */
	  if (arg > argv + 1) {
	      strcat(cmdLine, " ");
	  }
	  strcat(cmdLine, *arg);
      }

Comment 8

[Mike Kaply [:mkaply]]

Attachment: The correct fix

I apologize for the previous fix. I rushed through it.

Here is a fix that addresses everything mentioned and was compiled and tested
:)

Comment 9

[Wan-Teh Chang]

Comment on attachment 91498 [details] [diff] [review]
The correct fix

The local variables p, q, numBackslashes, and i are
now unused.

>+    *cmdLine = PR_MALLOC(cmdLineSize);

We should add code to handle the failure of PR_MALLOC.

      if (*cmdLine == NULL) {
	  return -1;
      }

>+    *cmdLine[0] = '\0';

I think this should be

      (*cmdLine)[0] = '\0';

In this particular case, they are the same,
but in general *cmdLine[i] and (*cmdLine)[i]
are different.

>+    for (arg = argv+1; *arg; arg++) {
>+        if (arg > argv +1) {
>+          strcat(*cmdLine, " ");
>         }

The indentation of the strcat call seems to be
wrong.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: OK, really, this time it's the correct fix

Sorry for removing the error checking.

I thought in your previous comment you had asked to remove it .

Comment 11

[Wan-Teh Chang]

Comment on attachment 91508 [details] [diff] [review]
OK, really, this time it's the correct fix

r=wtc.

Yes, it was my mistake to ask you to remove the
error check code.  I only saw the use of 'p' and
didn't pay attention to what the code was for. :-)

Comment 12

[Mike Kaply [:mkaply]]

wtc, is Mozilla 1.0 NSPR pulling from the MOZILLA_1_0_BRANCH or from a static 
tag for the shipped NSPR?

I'm wondering if this can go on the branch.

Thanks

Comment 13

[Wan-Teh Chang]

Mozilla 1.0 NSPR is pulling from the the MOZILLA_1_0_BRANCH.

Would you like to get drivers' approval?  It may be too late
for mozilla1.0.1.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 91508 [details] [diff] [review]
OK, really, this time it's the correct fix

a=asa (on behalf of drivers) for checkin to 1.1

Comment 15

[Wan-Teh Chang]

Fix has been checked into the tip and NSPRPUB_PRE_4_2_CLIENT_BRANCH
(used by the Mozilla trunk build) of NSPR.

Comment 16

[Mike Kaply [:mkaply]]

Fix checked into branch.

I'm opening a separate bug to address the helper app stuff specifically.

Comment 17

[Mike Kaply [:mkaply]]

Verified - all helper app space issues have been resolved.