Summary: | Image decoders must have private constructors to avoid refcount misuse: ASSERTION FAILED: m_deletionHasBegun when destroying ImageDecoder | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebCore Misc. | Assignee: | Miguel Gomez <magomez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, commit-queue, magomez, mcatanzaro, sabouhallawa | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=171205 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-04-23 20:32:17 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. 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. 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. 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. Created attachment 307975 [details]
Patch
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 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 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 > 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. (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. Created attachment 308078 [details]
Patch
Comment on attachment 308078 [details] Patch Clearing flags on attachment: 308078 Committed r215729: <http://trac.webkit.org/changeset/215729> All reviewed patches have been landed. Closing bug. Awesome, thanks! |