Bug 171211 – Image decoders must have private constructors to avoid refcount misuse: ASSERTION FAILED: m_deletionHasBegun when destroying ImageDecoder

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

Created attachment 307949 [details]
backtrace

When loading http://midori-browser.org/

ASSERTION FAILED: m_deletionHasBegun
../../Source/WTF/wtf/RefCounted.h(84) : WTF::RefCountedBase::~RefCountedBase()

Backtrace attached

[Michael Catanzaro]

Note that this is a *UI process* assertion. Apparently image decoders are used in the UI process for the favicon database. This is surprising and unexpected to me, since I would not expect this security-critical code to be running in the UI process. I suppose we need to render those favicons somehow, but it should happen in a constrained secondary process where only the result is passed back to the UI process. This is what we do in gdk-pixbuf now, so that the image decoders can be sandboxed with seccomp filters. Of course, that's a much larger challenge for another bug report... in this bug we should just fix this assertion.

[Michael Catanzaro]

So the problem here is that the PNGImageDecoder is being deleted before its refcount has fallen to zero, because it's being stored in a std::unique_ptr. That's wrong, it needs to be stored in a Ref or RefPtr.

[Michael Catanzaro]

Also note that this was easily-preventable, it's why we require that refcounted objects have create functions and private constructors. It looks like all the image decoder classes violate this rule. Fixing this bug thoroughly would entail not just replacing the use of unique_ptr with Ref or RefPtr, but also adding create functions to all the image decoders and making the constructors private in order to ensure this never happens again.

[Miguel Gomez]

Weird, I didn't know ICOImageDecoder would use a PNGImageDecoder inside. Anyway, we already have a create function in ImageDecoder that returns RefPtr, but ICOImageDecoder is not using it at the moment, but directly instantiates the PNGImageDecoder.

Changing ICOImageDecoder to use ImageDecoder::create shouldn't be too complicated. I'll give it a look as soon as I can.

[Miguel Gomez]

Created attachment 307975 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 307975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307975&action=review

To prevent this from happening again, I would make all constructors private to force everybody to use the create() functions.

> Source/WebCore/platform/graphics/ImageTypes.h:82
> +enum class ImageFormat {
> +    ImageFormatGIF,
> +    ImageFormatPNG,
> +    ImageFormatICO,
> +    ImageFormatCUR,
> +    ImageFormatJPEG,
> +    ImageFormatWEBP,
> +    ImageFormatBMP

When using enum class, including the class name in the elements is redundant, ImageFormat::GIF, ImageFormat::PNG looks better, IMO.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149
> +    default:
> +        return nullptr;

This should never happen, I think we should return the default and make this return a Ref<>

[Michael Catanzaro]

Comment on attachment 307975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307975&action=review

Thanks Miguel!

Yeah, private constructors please.

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149
>> +        return nullptr;
> 
> This should never happen, I think we should return the default and make this return a Ref<>

What do you mean by "return the default"... is there a default image decoder? I would do RELEASE_ASSERT_NOT_REACHED().

> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.h:142
> +        typedef Vector<RefPtr<ImageDecoder>> PNGDecoders;

So if you take Carlos's suggestion, then this should use Ref rather than RefPtr.

[Carlos Garcia Campos]

Comment on attachment 307975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307975&action=review

>>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149
>>> +        return nullptr;
>> 
>> This should never happen, I think we should return the default and make this return a Ref<>
> 
> What do you mean by "return the default"... is there a default image decoder? I would do RELEASE_ASSERT_NOT_REACHED().

Oops, sorry I meant remove the default

[Miguel Gomez]

> To prevent this from happening again, I would make all constructors private
> to force everybody to use the create() functions.

Sure, I'll change that.
 
> > Source/WebCore/platform/graphics/ImageTypes.h:82
> > +enum class ImageFormat {
> > +    ImageFormatGIF,
> > +    ImageFormatPNG,
> > +    ImageFormatICO,
> > +    ImageFormatCUR,
> > +    ImageFormatJPEG,
> > +    ImageFormatWEBP,
> > +    ImageFormatBMP
> 
> When using enum class, including the class name in the elements is
> redundant, ImageFormat::GIF, ImageFormat::PNG looks better, IMO.

Indeed, and I had that until the style checker started to complain, that's why I added the ImageFormat thing.

> > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149
> > +    default:
> > +        return nullptr;
> 
> This should never happen, I think we should return the default and make this
> return a Ref<>

mmmm actually Michael is right and there's no default, so an assertion makes more sense.

[Carlos Garcia Campos]

(In reply to Miguel Gomez from comment #9)
> > To prevent this from happening again, I would make all constructors private
> > to force everybody to use the create() functions.
> 
> Sure, I'll change that.
>  
> > > Source/WebCore/platform/graphics/ImageTypes.h:82
> > > +enum class ImageFormat {
> > > +    ImageFormatGIF,
> > > +    ImageFormatPNG,
> > > +    ImageFormatICO,
> > > +    ImageFormatCUR,
> > > +    ImageFormatJPEG,
> > > +    ImageFormatWEBP,
> > > +    ImageFormatBMP
> > 
> > When using enum class, including the class name in the elements is
> > redundant, ImageFormat::GIF, ImageFormat::PNG looks better, IMO.
> 
> Indeed, and I had that until the style checker started to complain, that's
> why I added the ImageFormat thing.
> 
> > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149
> > > +    default:
> > > +        return nullptr;
> > 
> > This should never happen, I think we should return the default and make this
> > return a Ref<>
> 
> mmmm actually Michael is right and there's no default, so an assertion makes
> more sense.

I meant remove the default, because there's no default. If you remove the default it will fail at compile time if new type is added.

[Miguel Gomez]

Created attachment 308078 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308078 [details]
Patch

Clearing flags on attachment: 308078

Committed r215729: <http://trac.webkit.org/changeset/215729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

Awesome, thanks!