RESOLVED FIXED 29980
Loading invalid image crashes in RenderImage::setImageSizeForAltText
https://bugs.webkit.org/show_bug.cgi?id=29980
Summary Loading invalid image crashes in RenderImage::setImageSizeForAltText
Yong Li
Reported 2009-10-01 13:58:38 PDT
Loading an invalid image will result crash in RenderImage::setImageSizeForAltText() When there's no image decoder found for the image, CachedImage::error() deletes the BitmapImage object. The following code crashes in this case: if (newImage) { // imageSize() returns 0 for the error image. We need the true size of the // error image, so we have to get it by grabbing image() directly. imageWidth += newImage->image()->width() * style()->effectiveZoom(); the fix should be: if (newImage && newImage->image()) { patch is following
Attachments
the patch (1.13 KB, patch)
2009-10-01 14:02 PDT, Yong Li
eric: review-
the test case (2.18 KB, patch)
2009-10-02 15:54 PDT, Yong Li
eric: review-
patch with test case (3.04 KB, patch)
2009-10-02 17:47 PDT, Yong Li
adele: review+
Yong Li
Comment 1 2009-10-01 14:02:37 PDT
Created attachment 40475 [details] the patch the patch
Darin Adler
Comment 2 2009-10-01 14:07:54 PDT
Can we construct a test case for this?
Yong Li
Comment 3 2009-10-01 15:00:59 PDT
(In reply to comment #2) > Can we construct a test case for this? I met the crash because I used a html file as .jpg. This is exactly my test case <html> <body> <img src=test.jpg></img> </body> </html> where test.jpg is actually a html file.
Darin Adler
Comment 4 2009-10-01 15:02:56 PDT
(In reply to comment #3) > I met the crash because I used a html file as .jpg. > > This is exactly my test case > <html> > <body> > <img src=test.jpg></img> > </body> > </html> > > where test.jpg is actually a html file. Great! That sounds like something that can be turned into an automated regression test. And we should.
David Kilzer (:ddkilzer)
Comment 5 2009-10-01 17:33:09 PDT
Darin Adler
Comment 6 2009-10-01 17:35:18 PDT
Since this patch fixes a null dereference, I don't think this is actually a security bug. Null dereferences are rarely exploitable, so this is just another crash -- we don't consider all crashes that a website can trigger to be security bugs.
Yong Li
Comment 7 2009-10-01 18:53:02 PDT
(In reply to comment #6) > Since this patch fixes a null dereference, I don't think this is actually a > security bug. Null dereferences are rarely exploitable, so this is just another > crash -- we don't consider all crashes that a website can trigger to be > security bugs. Ha, good to know
Eric Seidel (no email)
Comment 8 2009-10-02 12:48:30 PDT
Comment on attachment 40475 [details] the patch This change needs a LayoutTest.
Eric Seidel (no email)
Comment 9 2009-10-02 12:50:32 PDT
Removing security flag based on Darin's comment above. I agree, this is just a null dereference and not exploitable.
Yong Li
Comment 10 2009-10-02 15:28:01 PDT
(In reply to comment #8) > (From update of attachment 40475 [details]) > This change needs a LayoutTest. Which folder should I upload the test case to?
Eric Seidel (no email)
Comment 11 2009-10-02 15:35:09 PDT
I'd probably put it in fast/images. But you can put it whereever you like really. So long as you think the location makes sense or can justify it. :)
Yong Li
Comment 12 2009-10-02 15:35:56 PDT
(In reply to comment #11) > I'd probably put it in fast/images. But you can put it whereever you like > really. So long as you think the location makes sense or can justify it. :) fast/images is good
Yong Li
Comment 13 2009-10-02 15:54:20 PDT
Created attachment 40555 [details] the test case
Eric Seidel (no email)
Comment 14 2009-10-02 15:57:05 PDT
Comment on attachment 40555 [details] the test case These patches should all be one. invalid.jpg should be simpler (no need to be a full html page, or?) You don't actually need all this extra HTML boilerplate: +<html> +<head> + <title></title> You just need a little text to describe and the <img>, you file does not need to be valid HTML, it's better to be concise than valid.
Yong Li
Comment 15 2009-10-02 17:37:25 PDT
(In reply to comment #14) > (From update of attachment 40555 [details]) > These patches should all be one. invalid.jpg should be simpler (no need to be > a full html page, or?) > > You don't actually need all this extra HTML boilerplate: > +<html> > +<head> > + <title></title> > > You just need a little text to describe and the <img>, you file does not need > to be valid HTML, it's better to be concise than valid. but does it matter?
Yong Li
Comment 16 2009-10-02 17:47:42 PDT
Created attachment 40559 [details] patch with test case
Eric Seidel (no email)
Comment 17 2009-10-03 01:05:04 PDT
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 40555 [details] [details]) > > These patches should all be one. invalid.jpg should be simpler (no need to be > > a full html page, or?) > > > > You don't actually need all this extra HTML boilerplate: > > +<html> > > +<head> > > + <title></title> > > > > You just need a little text to describe and the <img>, you file does not need > > to be valid HTML, it's better to be concise than valid. > > but does it matter? Sure it matters! Needless verbosity infringes on communication. Being concise means being clear.
Adele Peterson
Comment 18 2009-10-13 17:06:52 PDT
Comment on attachment 40559 [details] patch with test case r=me. Please remove the empty <title> before committing though.
Yong Li
Comment 19 2009-10-14 08:49:50 PDT
Landed @ r49562
Note You need to log in before you can comment on or make changes to this bug.