bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

FMM (Free Memory Mismatch) in nsHTMLContentSinkStream

VERIFIED FIXED in M18

Status

()

Core
HTML: Parser
P2
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Daniel Bratell, Assigned: Akkana Peck)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3+]FIX IN HAND)

(Reporter)

Description

18 years ago
There is a Free Memory Mismatch in nsHTMLContentSinkStream reported by purify. 
Memory is allocated with new[] in EnsureBufferSize(int)

    mBuffer = new char[mBufferSize];

and then returned with nsMemory::Free (equal to PR_Free and free right now)in 
the destructor

    nsMemory::Free(mBuffer);

Either new[]/delete[] or nsMemory::Alloc/nsMemory::Free should be used.


(Note from Purify: Sometimes mismatched frees work without crashing. However, 
you should watch out for changes in the implementation of any of the allocation 
modules (for example, between compilers or versions of operating systems) that 
might cause problems.)

Comment 1

18 years ago
Triaging Rickg's list:

Akkana's area. 
Assignee: rickg → akkana
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M18

Comment 2

18 years ago
moving to m19
Keywords: correctness
Target Milestone: M18 → M19
(Reporter)

Comment 3

18 years ago
So I did this today, since it was so trivial. Can I get an approval to check 
this in? (Patch below)

Index: nsHTMLContentSinkStream.cpp
===================================================================
RCS file: /cvsroot/mozilla/htmlparser/src/nsHTMLContentSinkStream.cpp,v
retrieving revision 3.106
diff -r3.106 nsHTMLContentSinkStream.cpp
264c264
<     if(mBuffer) delete [] mBuffer;
---
>     if(mBuffer) nsMemory::Free(mBuffer);
267c267
<     mBuffer = new char[mBufferSize];
---
>     mBuffer = NS_STATIC_CAST(char *, nsMemory::Alloc(mBufferSize));
Keywords: approval
Whiteboard: FIX IN HAND

Comment 4

18 years ago
A more minimal fix would be to just change the dtor to "delete[] mBuffer". 
(delete[] is implemented to check for null, so the checked delete[] in 
EnsureBufferSize() should just call delete[] directly.)

Do one or the other: akkana appears to have written this code, so I'll let her 
pick whether she wants to use nsIAllocator or the C++ allocator.
(Reporter)

Comment 5

18 years ago
Yes, but it's only one more line and I thought nsMemory was preferred to normal 
new/delete. But Akkana, it's your call.

Comment 6

18 years ago
reviewed by Bijal and beppe, setting to p2, nsbeta3+
Priority: P3 → P2
Whiteboard: FIX IN HAND → [nsbeta3+]FIX IN HAND
(Assignee)

Comment 7

18 years ago
Fixed -- I checked in Daniel's version of the patch, so we won't have to worry
about the C++ destructor's definition changing.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

18 years ago
Sorry for the spam!  But apparently all these closed bugs need to have their
target milestones changed since M19 and M20 are going away.  Since they're
allready closed, I'm choosing M18.
Target Milestone: M19 → M18

Comment 9

18 years ago
updated qa contact.
QA Contact: janc → bsharma

Comment 10

18 years ago
Verified on:
build: 2001-03-29-09-Mtrunk
Platform: Win NT

Marked verified as per the above developer comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.