PR_REALLOC is misused in [@ ExpandMonitorCache]



14 years ago
10 years ago


(Reporter: timeless, Assigned: timeless)


({coverity, crash})

Firefox Tracking Flags

(Not tracked)


(crash signature, )


(1 attachment, 1 obsolete attachment)

Oh well. I had thought i did an audit of all realloc calls, i missed a spot :(~.

the behavior of realloc is:
1. if there's not enough memory available, return 0 and leave the in pointer alone
2. if there's not enough memory available in place, return a new pointer with the data moved and the old pointer freed.
3. if there's enough memory availble in place, return the old pointer.

case 1 is mostly covered, and case 3 is assumed. the code falls victim to not handling case 2.
Posted patch properly use realloc (obsolete) — Splinter Review
Attachment #218624 - Flags: review?(wtchang)
Comment on attachment 218624 [details] [diff] [review]
properly use realloc


>         p = (MonitorCacheEntry*)
>             PR_REALLOC(new_entries, added * sizeof(MonitorCacheEntry));
>         if (p == 0) {
>             /*
>             ** Total lossage. We just leaked a bunch of system monitors
>             ** all over the floor. This should never ever happen.
>             */
>             PR_ASSERT(p != 0);
>             return PR_FAILURE;
>         }
>+        new_entries = p;

The comment in the "if (p == 0)" block is wrong.
At that point, new_entries still has the pointers
to the system monitors.  So we can destroy those
monitors and free new_entries before returning
PR_FAILURE to avoid leading the system monitors
and memory.  Alternatively, we can continue with
the larger-than-necessary new_entries array.
Attachment #218624 - Flags: review?(wtchang) → review+
Comment on attachment 281597 [details] [diff] [review]
patch v2

This patch addresses Wan-Teh's previous review comments.  Wan-Teh, please review.
Attachment #281597 - Attachment is patch: true
Attachment #281597 - Attachment mime type: application/octet-stream → text/plain
Attachment #281597 - Flags: review?(wtc)
The patch attached to bug 95829, which is awaiting review, addresses 
this issue (and others) in a different way.  If we take that patch, then 
the patches attached to this bug are moot.  I have not reviewed that 
other patch thoroughly, but if it works, it not only solves this problem,
but fixes some nasty leaks also, so it seems preferable.
Comment on attachment 281597 [details] [diff] [review]
patch v2

Attachment #281597 - Flags: review?(wtc) → review+
Attachment #218624 - Attachment is obsolete: true
did this ever land?  it would be good to close or get off the security radar.
There are two positively-reviewed patches that both fix this bug.
One is attached to this bug.  The other is attached to bug 95829.
The patch attached to this bug fixes only the realloc bug.
The patch attached to bug 95829 also addresses some memory leaks.
Only one should be committed.
I think Wan-Teh needs to decide which one to commit, and commit it.

Wan-Teh, please do that.
i can't land things because i'm not a member of nspr...
Summary: PR_REALLOC is missused in [@ ExpandMonitorCache] → PR_REALLOC is misused in [@ ExpandMonitorCache]
Fixed by the patch in bug 95829 comment 20.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
Group: core-security
Crash Signature: [@ ExpandMonitorCache]
You need to log in before you can comment on or make changes to this bug.