Closed Bug 1277122 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: njn, Assigned: njn)

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.
https://hg.mozilla.org/mozilla-central/rev/50e32be65521
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.