238563 - Possible errors in NSPR code found with automated tool

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Philip K. Warren]

I have run a new automated testing tool developed internally on the NSPR source
code, and it has discovered a few problems. I have attempted to remove all of
the invalid ones. I will attach the output of the tool to this bug.

Comment 1

[Philip K. Warren]

Attachment: Error log

Comment 2

[Wan-Teh Chang]

Comment on attachment 144695 [details]
Error log

>"pr/src/malloc/prmem.c", line 378: memory leak

The tool is right.

>"pr/src/misc/prcountr.c", line 313: uninitialized `(rh)'

The tool is right.

>"pr/src/misc/prdtoa.c", line 580: Result of `!' operator is Boolean and that is used in a bitwise operation &

The tool is right.  It's not clear how to fix it.  I
will contact the author of dtoa.c.  I suspect the
fix is
    !x

Since !x is either 0 or 1, we have
    !x & 1
always equals
    !x

>"pr/src/misc/prdtoa.c", line 2420: uninitialized `spec_case'

I believe the tool is right and the fix is to initialize
spec_case to 0, but the function in question is very
complicated, so I also need to contact the author of dtoa.c.

>"pr/src/misc/prinit.c", line 641: invalid operation  involving NULL pointer `attr->fdInheritBuffer'
>ONE POSSIBLE PATH LEADING TO THE ERROR:
> "prinit.c", line 598: the if-condition is false
> "prinit.c", line 605: the if-condition is false
> "prinit.c", line 614: the if-condition is true
> "prinit.c", line 623: the if-condition is false
> "prinit.c", line 641: invalid operation  involving NULL pointer `attr->fdInheritBuffer'

This is a false alarm.	The if-condition at line 623
must be true if attr->fdInheritBuffer is NULL because
attr->fdInheritBufferSize is 0.  This can only be
inferred by looking at multiple functions and is
apparently beyond the ability of the tool.

>"pr/src/misc/prtpool.c", line 359: uninitialized `polljobs'
>ONE POSSIBLE PATH LEADING TO THE ERROR:
> "prtpool.c", line 314: allocating `polljobs'
> "prtpool.c", line 325: entering the loop for the 1st time
> "prtpool.c", line 329: the if-condition is false
> "prtpool.c", line 359: getting the value of `polljobs'

This is a false alarm, similar to the one above.  The
if-condition at line 329 must be true when we enter the
loop at 325 for the first time because tp->ioq.cnt and
tp->ioq.npollfds are both 0.  Again this can only be
inferred by looking at multiple functions and is
apparently beyond the ability of the tool.

>"pr/src/misc/prtpool.c", line 356: uninitialized `pollfds'
>ONE POSSIBLE PATH LEADING TO THE ERROR:
> "prtpool.c", line 313: allocating `pollfds'
> "prtpool.c", line 325: entering the loop for the 1st time
> "prtpool.c", line 329: the if-condition is false
> "prtpool.c", line 356: getting the value of `pollfds'

See above.

>"pr/src/misc/prtrace.c", line 598: uninitialized `(rh)'

The tool is right.

>"pr/src/threads/prtpd.c", line 192: passing NULL to argument 2 of `memcpy'

The tool is right.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch (checked in, except the prmem.c change)

Philip, please try this patch and see if it eliminates
all the errors (except the three false alarms in prinit.c
and prtpool.c).

Comment 4

[Philip K. Warren]

I have applied this patch and it does fix all of the valid problems you have
identified.

Comment 5

[Wan-Teh Chang]

Comment on attachment 144704 [details] [diff] [review]
Proposed patch (checked in, except the prmem.c change)

The changes in this patch, except the change to prmem.c,
have been checked into the NSPR trunk (NSPR 4.6) and
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 alpha).

I will come up with a better patch for prmem.c.

Comment 6

[Wan-Teh Chang]

Comment on attachment 144704 [details] [diff] [review]
Proposed patch (checked in, except the prmem.c change)

In the prmem.c change in this patch, the 'newptr'
variable is a no-op.  What was I thinking? I will
attach a new patch for prmem.c.

The new patch is the same as the prmem.c change in
this patch, with 'newptr' removed. Unfortunately, I
couldn't come up with a better patch as I promised.

Comment 7

[Wan-Teh Chang]

Attachment: prmem.c patch

The goal of this patch is to move the pr_ZoneMalloc(bytes)
call before the realloc(oldptr, bytes) call.  (I explained
why in a comment in the patch.)

You can convince yourself that this patch is correct
because it consists of the following code transformations.

1. Change the following code at the end of the function

       rv = pr_ZoneMalloc(bytes);
       if (rv) {
	   blah blah;
       }
       return rv;

  to
       rv = pr_ZoneMalloc(bytes);
       if (!rv) {
	   return rv;
       }
       blah blah;
       return rv;

2. Then, move the code

       rv = pr_ZoneMalloc(bytes);
       if (!rv) {
	   return rv;
       }

   up into both the "if" and "else" blocks above it.

3. In the "if" block, move that code further up before
the realloc(oldptr, bytes) call.

4. Finally, add the call

       pr_ZoneFree(rv);

   before we return from a failed realloc call.

Comment 8

[Darin Fisher]

Comment on attachment 176970 [details] [diff] [review]
prmem.c patch

This looks fine, r=darin.

But, I don't really understand why we have the fprintf following a PR_ASSERT. 
It seems like we can't reach this branch in a debug build.  Is it just there
for release builds or something like that?  Backwards compat?

Comment 9

[Wan-Teh Chang]

Thanks a lot for the code review, Darin.  I checked
in the prmem.c patch on the NSPR trunk (NSPR 4.6).

You are right, the PR_ASSERT and the DEBUG fprintf
can't both be used.  They are also redundant with
each other.  We should only keep one.

I removed the PR_ASSERT from pr_ZoneRealloc and kept
the DEBUG fprintf to match the code in pr_ZoneFree.

Comment 10

[Wan-Teh Chang]

The patch has been checked into the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta 2)
as well.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176970 [details] [diff] [review]
prmem.c patch

sr=nelson

I might suggest the addition of a comment to this line in pr_ZoneRealloc

>         oldptr = realloc(oldptr, bytes);

That looks like an obvious leak of oldptr (when realloc returns NULL),
but in this case (since this function is itself a realloc function) 
that is not a leak unless the caller of this function likewise loses
the old pointer when NULL is returned.

Comment 12

[Wan-Teh Chang]

Attachment: Add the comment that Nelson suggested

The comment reads:

	 /* We don't know how big it is.  But we can fix that. */
	 oldptr = realloc(oldptr, bytes);
+	 /*
+	  * If realloc returns NULL, this function loses the original
+	  * value of oldptr.  This isn't a leak because the caller of
+	  * this function still has the original value of oldptr.
+	  */
	 if (!oldptr) {