Bug 29980

Summary:

Loading invalid image crashes in RenderImage::setImageSizeForAltText

Product:

WebKit

Reporter:

Yong Li <yong.li.webkit>

Component:

Images

Assignee:

Yong Li <yong.li.webkit>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric, staikos

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
the patch	eric: review-
the test case	eric: review-
patch with test case	adele: review+

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-	Details Formatted Diff Diff
the test case (2.18 KB, patch) 2009-10-02 15:54 PDT, Yong Li	eric: review-	Details Formatted Diff Diff
patch with test case (3.04 KB, patch) 2009-10-02 17:47 PDT, Yong Li	adele: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

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

<rdar://problem/7269951>

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.