We leak oldVal on OOM in nsUint32ToContentHashEntry::PutContent

RESOLVED FIXED

Status

()

Core
DOM
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

({coverity, memory-leak})

Trunk
coverity, memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
The call to GetContent could return null, in which case NS_RELEASE dereference null.
(Assignee)

Comment 1

12 years ago
Created attachment 218127 [details] [diff] [review]
patch
Attachment #218127 - Flags: superreview?(roc)
Attachment #218127 - Flags: review?(roc)
> The call to GetContent could return null

Not really.  The code is:

 186                       if (GetContent()) {
 187                         nsIContent* oldVal = GetContent();

Now perhaps we want to only call GetContent() once and then just test oldVal?  Would probably make coverity happier and mean only two conditional tests there instead of the current 3....

I'd really have expected coverity to not flag this, though.  Is there a way to report to them that this is bogus?  Does flagging the report as "FALSE" in scan.coverity.com do that?
Attachment #218127 - Flags: superreview?(roc)
Attachment #218127 - Flags: superreview-
Attachment #218127 - Flags: review?(roc)
Attachment #218127 - Flags: review-
(Assignee)

Comment 3

12 years ago
Created attachment 218133 [details] [diff] [review]
patch for leak

True, I should have noticed that. :-/ Nevertheless, there's still a leak on OOM that we should fix.
Attachment #218127 - Attachment is obsolete: true
Attachment #218133 - Flags: superreview?(roc)
Attachment #218133 - Flags: review?(roc)
(Assignee)

Comment 4

12 years ago
I don't know if there's a special way to report false positives to coverity. I'd hope they keep looking at the reports marked FALSE though to improve their code.
Keywords: crash → mlk
Summary: Null dereference in nsUint32ToContentHashEntry::PutContent → Leak oldVal on OOM in nsUint32ToContentHashEntry::PutContent
Comment on attachment 218133 [details] [diff] [review]
patch for leak

SetContent addrefs, so this will double-addref the content and leak.
Attachment #218133 - Flags: superreview?(roc)
Attachment #218133 - Flags: superreview-
Attachment #218133 - Flags: review?(roc)
Attachment #218133 - Flags: review-
Comment on attachment 218133 [details] [diff] [review]
patch for leak

Never mind me.  I need to learn to read.
Attachment #218133 - Flags: superreview-
Attachment #218133 - Flags: superreview+
Attachment #218133 - Flags: review-
Attachment #218133 - Flags: review+

Comment 7

12 years ago
fwiw they do read through FALSEs, but if there's a specific pattern we want them to learn, we're supposed to email something.
(Assignee)

Comment 8

12 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: Leak oldVal on OOM in nsUint32ToContentHashEntry::PutContent → We leak oldVal on OOM in nsUint32ToContentHashEntry::PutContent
You need to log in before you can comment on or make changes to this bug.