334285 - PR_REALLOC is misused in [@ ExpandMonitorCache]

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[timeless]

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.

Comment 1

[timeless]

Attachment: properly use realloc

Comment 2

[Wan-Teh Chang]

Comment on attachment 218624 [details] [diff] [review]
properly use realloc

r=wtc.

>         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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281597 [details] [diff] [review]
patch v2

This patch addresses Wan-Teh's previous review comments.  Wan-Teh, please review.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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 6

[Wan-Teh Chang]

Comment on attachment 281597 [details] [diff] [review]
patch v2

r=wtc.

Comment 7

[chris hofmann]

did this ever land?  it would be good to close or get off the security radar.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[timeless]

i can't land things because i'm not a member of nspr...

Comment 10

[Wan-Teh Chang]

Fixed by the patch in bug 95829 comment 20.