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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

pixman_image_create_bits() is fallible and will return null on failure. Most call sites null-check, but three don't.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add missing null checks for pixman_image_create_bits()

Comment 2

[Nicholas Nethercote [inactive]]

I forgot to say: seth, please review the image/ change; lsalzman, please review the gfx/ change.

Comment 3

[Lee Salzman [:lsalzman]]

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/.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Add missing null checks for pixman_image_create_bits()

Addresses lsalzman's comment.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

> > +  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.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50e32be6552151fd71dd828b30b24749b5fbf4d4
Bug 1277122 - Add missing null checks for pixman_image_create_bits(). r=seth,lsalzman.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50e32be65521