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
Created attachment 40475 [details] the patch the patch
Can we construct a test case for this?
(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.
(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.
<rdar://problem/7269951>
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.
(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
Comment on attachment 40475 [details] the patch This change needs a LayoutTest.
Removing security flag based on Darin's comment above. I agree, this is just a null dereference and not exploitable.
(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?
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. :)
(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
Created attachment 40555 [details] the test case
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.
(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?
Created attachment 40559 [details] patch with test case
(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.
Comment on attachment 40559 [details] patch with test case r=me. Please remove the empty <title> before committing though.
Landed @ r49562