Closed Bug 238563 Opened 21 years ago Closed 20 years ago

Possible errors in NSPR code found with automated tool

Categories

(NSPR :: NSPR, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pkwarren, Assigned: wtc)

Details

Attachments

(4 files)

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.
Attached file Error log
Status: NEW → ASSIGNED
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.
Philip, please try this patch and see if it eliminates all the errors (except the three false alarms in prinit.c and prtpool.c).
Attachment #144704 - Flags: review?(darin)
I have applied this patch and it does fix all of the valid problems you have identified.
Attachment #144704 - Flags: review?(darin) → review+
Target Milestone: --- → 4.6
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 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.
Attachment #144704 - Attachment description: Proposed patch → Proposed patch (checked in, except the prmem.c change)
Attached patch prmem.c patchSplinter Review
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.
Attachment #176970 - Flags: superreview?(nelson)
Attachment #176970 - Flags: review?(darin)
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?
Attachment #176970 - Flags: review?(darin) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
The patch has been checked into the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta 2) as well.
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.
Attachment #176970 - Flags: superreview?(nelson) → superreview+
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) {
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: