176543 - Malloc of 0 and bad initialization causes crash

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Mike Kaply [:mkaply]]

We were trying to run an XPI that starts an app and got a crash in NSPR.

Tracked it down to two issues.

1. We were failing on a malloc of 0.

2. We weren't initializing a variable, so the free was crashing.

Diff attached.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Fix for problem

Handle malloc of 0 and properly initialize newenvp

Comment 2

[Wan-Teh Chang]

How can you have a command line size of 0?  It seems
that that should be treated as an error.

Comment 3

[Mike Kaply [:mkaply]]

command line is the arguments passed to the application.

In this case, it is simply invoking an application with no arguments.

Comment 4

[Wan-Teh Chang]

Mike,

I think you are using PR_CreateProcess incorrectly.

By convention, the first argument in the argv array
is the name of the application you want to invoke.
So a command line size of 0 is an error.

See the PR_CreateProcess documentation at
http://www.mozilla.org/projects/nspr/reference/html/prprocess.html#24535.

The second "hunk" of your patch is good though.  It's
just that the first "hunk" of your patch shows that
you seem to be using PR_CreateProcess incorrectly and
if commandLineSize is 0 (which will not happen under
normal circumstances), we should return -1 (a failure),
not 0.

Comment 5

[Mike Kaply [:mkaply]]

The OS/2 function we call take the argv string directly.

You pass argv[0] (the program name) in one place, and the rest of the arguments
in another.

The assembleCmdLine function is designed to take argv[1] and greater and create
the parameters to pass to the application.

for (arg = argv+1; *arg; arg++) {
    cmdLineSize += strlen(*arg) + 1; /* space in between, or final null */
}

So if there are no arguments, this function will create a 0 length command line.

The function is named poorly. It should be assembleParameters or something like
that.

Comment 6

[Wan-Teh Chang]

I didn't realize that.  Thanks for the explanation.

I think there is still something wrong with your
patch.  If there are no arguments, cmdLine will be
NULL, as opposed to an empty string.  It seems that
cmdLine should be an empty string, which means you
want cmdLineSize to be 1 (for the terminating null
byte) if there are no arguments.

So I think the fix should be something like:

     for (arg = argv+1; *arg; arg++) {
         cmdLineSize += strlen(*arg) + 1; /* space in between, or final null */
     }
+    if (cmdLineSize == 0) { /* no arguments */
+        cmdLineSize = 1; /* final null */
+    }
     *cmdLine = PR_MALLOC(cmdLineSize);
     if (*cmdLine == NULL) {
         return -1;

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Better fix

Better patch per WTC.

Add one to cndLine size so we always allocate at least one.

This allows other code (like strlen(cmdLine)) to work.

Comment 8

[Wan-Teh Chang]

Attachment: Better fix v3

I think it's clearer to initialize cmdLineSize to 1
instead of adding 1 to it.

Comment 9

[Mike Kaply [:mkaply]]

Comment on attachment 104583 [details] [diff] [review]
Better fix v3

Fine with me - r=mkaply

Comment 10

[Wan-Teh Chang]

Do you need this fix in Mozilla 1.2final?

Comment 11

[Mike Kaply [:mkaply]]

If I can get it there, I would like it to.

Comment 12

[Wan-Teh Chang]

If you get drivers' approval, I can check it in
for Mozilla 1.2 final.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 104583 [details] [diff] [review]
Better fix v3

a=roc+moz for trunk

Comment 14

[Wan-Teh Chang]

Patch checked into the trunk and NSPRPUB_PRE_4_2_CLIENT_BRANCH
of NSPR.