Closed Bug 1024226 Opened 6 years ago Closed 6 years ago

moz_gdk_pixbuf_to_channel leaks |buf| if |do_CreateInstance("@mozilla.org/io/string-input-stream;1", &rv);| fails

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mccr8, Assigned: wlitwinczyk, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, memory-leak, Whiteboard: [MemShrink:P3][CID 221162])

Attachments

(1 file, 2 obsolete files)

No description provided.
Whiteboard: [MemShrink][CID 221162] → [MemShrink:P3][CID 221162]
Mentor: continuation
Assignee: nobody → wlitwinczyk
Attached patch skia_bug_1024226 (obsolete) — Splinter Review
Hi Andrew,

Would you mind seeing if this patch fixes the leak?
Flags: needinfo?(continuation)
As I said in the other bug, I don't have a test case or anything, but it looks like it would fix the leak to me.
Flags: needinfo?(continuation)
Attachment #8453204 - Flags: review?(milan)
Comment on attachment 8453204 [details] [diff] [review]
skia_bug_1024226

Looks good to me.  Didn't check - NS_Free nulls out buf?  I know we return right away, but I don't like non-null pointers to freed memory :)
Attachment #8453204 - Flags: review?(milan)
Attachment #8453204 - Flags: review?(jmuizelaar)
Attachment #8453204 - Flags: feedback+
NS_Free appears to forward to regular free, so no zeroing of buf.

I was going to make the change, but it's declared as a read-only variable:

http://dxr.mozilla.org/mozilla-central/source/image/decoders/icon/gtk/nsIconChannel.cpp#86

in order to verify that the copying below worked correctly.
Comment on attachment 8453204 [details] [diff] [review]
skia_bug_1024226

Review of attachment 8453204 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/icon/gtk/nsIconChannel.cpp
@@ +132,1 @@
>    NS_ENSURE_SUCCESS(rv, rv);

You might as well move this return below the NS_Free(buf) instead of having us check rv again.

@@ +132,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    rv = stream->AdoptData((char*)buf, buf_size);
>    NS_ENSURE_SUCCESS(rv, rv);

This NS_ENSURE_SUCCESS should also be:

if (NS_FAILED(rv)) {
    NS_Free(buf);
    return;
}

to avoid leaking here.
Attachment #8453204 - Flags: review?(jmuizelaar) → review-
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8453204 [details] [diff] [review]
> skia_bug_1024226
> 
> Review of attachment 8453204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/decoders/icon/gtk/nsIconChannel.cpp
> @@ +132,1 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> You might as well move this return below the NS_Free(buf) instead of having
> us check rv again.
> 
> @@ +132,4 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    rv = stream->AdoptData((char*)buf, buf_size);
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> This NS_ENSURE_SUCCESS should also be:
> 
> if (NS_FAILED(rv)) {
>     NS_Free(buf);
>     return;
> }
This takes care of my comment as well.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8453204 [details] [diff] [review]
> skia_bug_1024226
> 
> Review of attachment 8453204 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +132,4 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    rv = stream->AdoptData((char*)buf, buf_size);
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> This NS_ENSURE_SUCCESS should also be:
> 
> if (NS_FAILED(rv)) {
>     NS_Free(buf);
>     return;
> }
> 
> to avoid leaking here.

I think it shouldn't be freed because AdoptData will transfer ownership to the stream:

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsIStringStream.idl#39
Attached patch leak_bug_1024226 (obsolete) — Splinter Review
Attachment #8453204 - Attachment is obsolete: true
Attachment #8457482 - Flags: review?(jmuizelaar)
(In reply to Walter Litwinczyk [:walter] from comment #8)
> 
> I think it shouldn't be freed because AdoptData will transfer ownership to
> the stream:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsIStringStream.idl#39

Does the ownership still transfer if AdoptData fails?
Flags: needinfo?(wlitwinczyk)
Attachment #8457482 - Flags: review?(jmuizelaar) → review+
(In reply to Walter Litwinczyk [:walter] from comment #11)
> It doesn't appear that AdoptData can fail:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#156
> http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.
> cpp#447

Maybe add a comment about that where we check for failure then
Attached patch leak_bug_1024226Splinter Review
I left the NS_ENSURE_SUCCESS() so it would print something in non-debug builds.
Attachment #8457482 - Attachment is obsolete: true
Attachment #8458146 - Flags: review?(jmuizelaar)
Attachment #8458146 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/4d8154a37ebb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.