333639 - PR_CreateProcess() function on OS/2 might not handle empty strings and strings with spaces as argv parameters

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Aleksey Sanin]

While investigating bug #333637 I noticed that assembleCmdLine() function for OS/2 is different from Windows function. I believe OS/2 version does not handle empty strings and strings with spaces as argv parameters correctly. However, I can not test it since I don't have OS/2 around.

Comment 1

[Wan-Teh Chang]

This bug may not be worth fixing.

Comment 2

[Aleksey Sanin]

Sure :) Though I believe the fix is trivial: copy assembleCmdLine() from Windows code into OS/2 code :)

Comment 3

[Peter Weilbacher]

I just wanted to try doing this and to check the results added a printf in _PR_CreateOS2Process. The output never appears, so I wonder if that functions (and hence assembleCmdLine) is called at all on OS/2. (I cannot really make sense through all the tricky redefines of function names myself...)

Comment 4

[Peter Weilbacher]

Oh, sorry. I was talking about trying it out within a SeaMonkey/Firefox trunk build.

Comment 5

[Peter Weilbacher]

Attachment: Sync assembleCmdLine() with ntmisc.c

OK, whatever the use of this function is on OS/2, this patch syncs it to be the same as in ntmisc.c. I don't see anything that won't work on OS/2 but as I said I don't know how to test it.

I also took the liberty to fix the two warnings (about unused pEnvWPS and type mismatch when calling DosScanEnv) I got on compiling os2misc.c.

Comment 6

[Peter Weilbacher]

I suspect that mkaply or somebody on his team wrote and tested the original function on OS/2. Mike, do you remember any reason why this was not originally synced with the Windows version?

Comment 7

[Mike Kaply [:mkaply]]

I vaguely remember having to do some extra stuff regarding embedded spaces and such on OS/2, but that might have been in another file.

I think the easiest way to test this is with helpers - create helpers with spaces in the names and spaces in the file names.

Comment 8

[Peter Weilbacher]

OK, calls of helpers indeed use assembleCmdLine function so I could test this. With the new version I synced from Windows it doesn't work any more. Several issues:

1. Filenames already get wrapped in nsMIMEInfoOS2::LaunchWithFile, so from the Mozilla point of view we are safe for filename with spaces. (Not sure if for other NSPR apps -- which don't exist on OS/2 AFAIK -- we would need to care about this.)

2. Perhaps it makes sense on Windows to escape " and \ but on OS/2 these do not seem to be valid filenames on the file system level and are always represented by "!" (even if they display as " and \ in the OS/2 UI).

3. The for loops in the Windows assembleCmdLine() start with argv instead of argv+1 as in the current os2misc.c. On OS/2 that would have the effect that the apps gets itself passed as its first parameter...

Comment 9

[Peter Weilbacher]

Attachment: Fix warnings and add note.

Instead of marking this bug invalid I think we can at least use this chance to fix the warnings about unused pEnvWPS and 2nd parameter to DosScanEnv and perhaps add a note about handling of spaces. Something like this patch.

Comment 10

[Wan-Teh Chang]

Comment on attachment 222752 [details] [diff] [review]
Fix warnings and add note.

Peter, could you ask an OS/2 developer to review
this patch?  I don't know what the PSZ type is,
so I don't know why "char **" needs to be cast
to "PSZ *".  I'm not sure if the comment is correct
either...

Comment 11

[Peter Weilbacher]

Comment on attachment 222752 [details] [diff] [review]
Fix warnings and add note.

Mike, any more comments/recommendations?

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 222752 [details] [diff] [review]
Fix warnings and add note.

Nope, this is pretty straightforward and I probably should have put a comment like that in when I wrote the code :)

Comment 13

[Peter Weilbacher]

Comment on attachment 222752 [details] [diff] [review]
Fix warnings and add note.

mkaply, wtc, could one of you please check this into NSPR, too? (I don't think this is needed on 1.8 branch.)

Comment 14

[Peter Weilbacher]

Patch checked into NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla trunk) and NSPR trunk.