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

RESOLVED FIXED in 4.7

Status

NSPR
NSPR
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Aleksey Sanin, Assigned: Peter Weilbacher)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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

12 years ago
This bug may not be worth fixing.
(Reporter)

Comment 2

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

Comment 3

12 years ago
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...)
(Assignee)

Comment 4

12 years ago
Oh, sorry. I was talking about trying it out within a SeaMonkey/Firefox trunk build.
(Assignee)

Comment 5

12 years ago
Created attachment 218267 [details] [diff] [review]
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.
Assignee: wtchang → mozilla
Status: NEW → ASSIGNED
Attachment #218267 - Flags: review?(wtchang)
(Assignee)

Comment 6

12 years ago
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

12 years ago
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.
(Assignee)

Comment 8

12 years ago
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...
(Assignee)

Comment 9

12 years ago
Created attachment 222752 [details] [diff] [review]
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.
Attachment #218267 - Attachment is obsolete: true
Attachment #222752 - Flags: review?(wtchang)
Attachment #218267 - Flags: review?(wtchang)

Comment 10

12 years ago
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...
Attachment #222752 - Flags: review?(wtchang) → review+
(Assignee)

Comment 11

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

Mike, any more comments/recommendations?
Attachment #222752 - Flags: superreview?(mozilla)

Comment 12

12 years ago
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 :)
Attachment #222752 - Flags: superreview?(mozilla) → superreview+
(Assignee)

Comment 13

11 years ago
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.)
(Assignee)

Comment 14

11 years ago
Patch checked into NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla trunk) and NSPR trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Target Milestone: --- → 4.7
You need to log in before you can comment on or make changes to this bug.