Closed
Bug 238563
Opened 21 years ago
Closed 20 years ago
Possible errors in NSPR code found with automated tool
Categories
(NSPR :: NSPR, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
4.6
People
(Reporter: pkwarren, Assigned: wtc)
Details
Attachments
(4 files)
3.62 KB,
text/plain
|
Details | |
4.65 KB,
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
1.81 KB,
patch
|
darin.moz
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
761 bytes,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•21 years ago
|
||
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.
Assignee | ||
Comment 3•21 years ago
|
||
Philip, please try this patch and see if it eliminates
all the errors (except the three false alarms in prinit.c
and prtpool.c).
Assignee | ||
Updated•21 years ago
|
Attachment #144704 -
Flags: review?(darin)
Reporter | ||
Comment 4•21 years ago
|
||
I have applied this patch and it does fix all of the valid problems you have
identified.
Updated•21 years ago
|
Attachment #144704 -
Flags: review?(darin) → review+
Assignee | ||
Updated•21 years ago
|
Target Milestone: --- → 4.6
Assignee | ||
Comment 5•21 years ago
|
||
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.
Assignee | ||
Comment 6•20 years ago
|
||
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)
Assignee | ||
Comment 7•20 years ago
|
||
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 8•20 years ago
|
||
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+
Assignee | ||
Comment 9•20 years ago
|
||
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
Assignee | ||
Comment 10•20 years ago
|
||
The patch has been checked into the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta 2)
as well.
Comment 11•20 years ago
|
||
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+
Assignee | ||
Comment 12•20 years ago
|
||
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.
Description
•