Bug 29980 - Loading invalid image crashes in RenderImage::setImageSizeForAltText
Summary: Loading invalid image crashes in RenderImage::setImageSizeForAltText
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yong Li
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-10-01 13:58 PDT by Yong Li
Modified: 2009-10-14 08:49 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 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
Comment 1 Yong Li 2009-10-01 14:02:37 PDT
Created attachment 40475 [details]
the patch

the patch
Comment 2 Darin Adler 2009-10-01 14:07:54 PDT
Can we construct a test case for this?
Comment 3 Yong Li 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.
Comment 4 Darin Adler 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.
Comment 5 David Kilzer (:ddkilzer) 2009-10-01 17:33:09 PDT
<rdar://problem/7269951>
Comment 6 Darin Adler 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.
Comment 7 Yong Li 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
Comment 8 Eric Seidel (no email) 2009-10-02 12:48:30 PDT
Comment on attachment 40475 [details]
the patch

This change needs a LayoutTest.
Comment 9 Eric Seidel (no email) 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.
Comment 10 Yong Li 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?
Comment 11 Eric Seidel (no email) 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. :)
Comment 12 Yong Li 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
Comment 13 Yong Li 2009-10-02 15:54:20 PDT
Created attachment 40555 [details]
the test case
Comment 14 Eric Seidel (no email) 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.
Comment 15 Yong Li 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?
Comment 16 Yong Li 2009-10-02 17:47:42 PDT
Created attachment 40559 [details]
patch with test case
Comment 17 Eric Seidel (no email) 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.
Comment 18 Adele Peterson 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.
Comment 19 Yong Li 2009-10-14 08:49:50 PDT
Landed @ r49562