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.
This bug may not be worth fixing.
Sure :) Though I believe the fix is trivial: copy assembleCmdLine() from Windows code into OS/2 code :)
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...)
Oh, sorry. I was talking about trying it out within a SeaMonkey/Firefox trunk build.
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.
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?
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.
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...
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.
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 on attachment 222752 [details] [diff] [review] Fix warnings and add note. Mike, any more comments/recommendations?
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 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.)
Patch checked into NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla trunk) and NSPR trunk.