Bug 171211 - Image decoders must have private constructors to avoid refcount misuse: ASSERTION FAILED: m_deletionHasBegun when destroying ImageDecoder
Summary: Image decoders must have private constructors to avoid refcount misuse: ASSER...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Miguel Gomez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-23 20:32 PDT by Michael Catanzaro
Modified: 2017-04-25 07:30 PDT (History)
6 users (show)

See Also:


Attachments
backtrace (15.77 KB, text/plain)
2017-04-23 20:32 PDT, Michael Catanzaro
no flags Details
Patch (6.67 KB, patch)
2017-04-24 07:37 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2017-04-25 02:30 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-04-23 20:32:17 PDT
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
Comment 1 Michael Catanzaro 2017-04-23 20:38:05 PDT
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.
Comment 2 Michael Catanzaro 2017-04-23 20:48:37 PDT
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.
Comment 3 Michael Catanzaro 2017-04-23 21:01:44 PDT
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.
Comment 4 Miguel Gomez 2017-04-24 00:32:39 PDT
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.
Comment 5 Miguel Gomez 2017-04-24 07:37:19 PDT
Created attachment 307975 [details]
Patch
Comment 6 Carlos Garcia Campos 2017-04-24 08:18:52 PDT
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<>
Comment 7 Michael Catanzaro 2017-04-24 08:55:02 PDT
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.
Comment 8 Carlos Garcia Campos 2017-04-24 09:08:08 PDT
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
Comment 9 Miguel Gomez 2017-04-24 09:10:53 PDT
> 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.
Comment 10 Carlos Garcia Campos 2017-04-24 09:22:25 PDT
(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.
Comment 11 Miguel Gomez 2017-04-25 02:30:48 PDT
Created attachment 308078 [details]
Patch
Comment 12 WebKit Commit Bot 2017-04-25 05:35:22 PDT
Comment on attachment 308078 [details]
Patch

Clearing flags on attachment: 308078

Committed r215729: <http://trac.webkit.org/changeset/215729>
Comment 13 WebKit Commit Bot 2017-04-25 05:35:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Michael Catanzaro 2017-04-25 07:30:47 PDT
Awesome, thanks!