Closed Bug 1277122 Opened 9 years ago Closed 9 years ago

[Static Analysis][Dereference Null Return Value] Missing null checks on some pixman_image_create_bits() calls

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1295437)

Attachments

(1 file, 1 obsolete file)

pixman_image_create_bits() is fallible and will return null on failure. Most call sites null-check, but three don't.
Attachment #8758507 - Flags: review?(seth)
Attachment #8758507 - Flags: review?(lsalzman)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
I forgot to say: seth, please review the image/ change; lsalzman, please review the gfx/ change.
Comment on attachment 8758507 [details] [diff] [review] Add missing null checks for pixman_image_create_bits() Review of attachment 8758507 [details] [diff] [review]: ----------------------------------------------------------------- This will leak pixman images if src succeeds but dst does not. You need to make sure you pixman_image_unref(src) in that case before returning. The same problem is repeated in both gfx/ and image/.
Attachment #8758507 - Flags: review?(lsalzman) → review-
Addresses lsalzman's comment.
Attachment #8758541 - Flags: review?(seth)
Attachment #8758541 - Flags: review?(lsalzman)
Attachment #8758507 - Attachment is obsolete: true
Attachment #8758507 - Flags: review?(seth)
Attachment #8758541 - Flags: review?(lsalzman) → review+
Comment on attachment 8758541 [details] [diff] [review] Add missing null checks for pixman_image_create_bits() Review of attachment 8758541 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: gfx/2d/DrawTargetCairo.cpp @@ +1967,5 @@ > pixman_image_create_bits(srcFormat, > srcSurf->GetSize().width, srcSurf->GetSize().height, > (uint32_t*)srcMap.GetData(), srcMap.GetStride()); > + if (!src) { > + pixman_image_unref(dst); It'd be lovely to have a RAII wrapper for this but this bug isn't really the place to do it. ::: image/FrameAnimator.cpp @@ +808,5 @@ > aSrcRect.width, aSrcRect.height, > reinterpret_cast<uint32_t*>(const_cast<uint8_t*>(aSrcData)), > aSrcRect.width * 4); > + if (!src) { > + return NS_ERROR_OUT_OF_MEMORY; We don't check return values, but what else is new? Shouldn't be problematic, anyway, as it's not clear there's any interesting recovery we could do.
Attachment #8758541 - Flags: review?(seth) → review+
> > + if (!src) { > > + pixman_image_unref(dst); > > It'd be lovely to have a RAII wrapper for this but this bug isn't really the > place to do it. Yeah... I see 70 instances of pixman_image_unref() in the code but only 4 of them are in C++ code so the benefit would be low.
https://hg.mozilla.org/integration/mozilla-inbound/rev/50e32be6552151fd71dd828b30b24749b5fbf4d4 Bug 1277122 - Add missing null checks for pixman_image_create_bits(). r=seth,lsalzman.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: