Bug 47683 - Update ImageDecoder::create to match sniffing spec
Summary: Update ImageDecoder::create to match sniffing spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-14 13:22 PDT by Adam Barth
Modified: 2011-05-23 12:48 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.67 KB, patch)
2010-10-14 13:25 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2010-10-14 14:35 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
the test case I was playing around with (for posterity) (1.51 KB, text/html)
2010-10-14 16:52 PDT, Eric Seidel (no email)
no flags Details
merge to TOT (6.18 KB, patch)
2010-11-02 16:42 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
merge to TOT (6.24 KB, patch)
2010-11-02 19:09 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (1.46 KB, patch)
2011-05-23 10:28 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.09 KB, patch)
2011-05-23 10:52 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-10-14 13:22:32 PDT
Update ImageDecoder::create to match sniffing spec
Comment 1 Adam Barth 2010-10-14 13:25:28 PDT
Created attachment 70769 [details]
Patch
Comment 2 WebKit Review Bot 2010-10-14 13:35:56 PDT
Attachment 70769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4436039
Comment 3 Peter Kasting 2010-10-14 13:36:43 PDT
Comment on attachment 70769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70769&action=review

Is walking a vector char-by-char really better than memcmp()?

LGTM otherwise

> WebCore/platform/image-decoders/ImageDecoder.cpp:58
> +    for (unsigned i = 0; i < pattern.length(); ++i) {

This is going to have trouble with the WEBP signature since that has a hole in the middle.  Do you have a plan for how you'll extend it?

> WebCore/platform/image-decoders/ImageDecoder.cpp:73
> +    DEFINE_STATIC_LOCAL(Vector<char>, GIFSignature1, ("GIF87a", 6));

Nit: I think it'd be a little clearer to move these to the top of the function and define maxSignatureLength next to them.

> WebCore/platform/image-decoders/ImageDecoder.cpp:79
> +    DEFINE_STATIC_LOCAL(Vector<char>, ICOSignature2, ("\x00\x00\x02\x00", 4));

Nit: I suggest "CURSignature" instead for clarity.

> WebCore/platform/image-decoders/ImageDecoder.cpp:81
> +    if (matcheSignature(GIFSignature1, contents) || matcheSignature(GIFSignature2, contents))

Won't compile: matcheSignature -> matchesSignature

> WebCore/platform/image-decoders/ImageDecoder.cpp:93
>      // ICOs always begin with a 2-byte 0 followed by a 2-byte 1.

Nit: These comments aren't really needed.
Comment 4 Eric Seidel (no email) 2010-10-14 13:40:10 PDT
Is this observable from JS?  could we create some bogus binary files which match these signatures and find out how the engine is sniffing them?
Comment 5 Peter Kasting 2010-10-14 13:41:59 PDT
(In reply to comment #4)
> Is this observable from JS?

Not as far as I know.  There's unlikely to even be theoretical impact given that things like the GIF signatures were already being checked in GIFImageDecoder.cpp.
Comment 6 WebKit Review Bot 2010-10-14 14:01:12 PDT
Attachment 70769 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4466044
Comment 7 Adam Barth 2010-10-14 14:05:30 PDT
> Is walking a vector char-by-char really better than memcmp()?

It's more obviously memory safe.

> > WebCore/platform/image-decoders/ImageDecoder.cpp:58
> > +    for (unsigned i = 0; i < pattern.length(); ++i) {
> 
> This is going to have trouble with the WEBP signature since that has a hole in the middle.  Do you have a plan for how you'll extend it?

I actually wrote the patch originally with a mask parameter too, but then I removed it because it's not needed yet.

> > WebCore/platform/image-decoders/ImageDecoder.cpp:73
> > +    DEFINE_STATIC_LOCAL(Vector<char>, GIFSignature1, ("GIF87a", 6));
> 
> Nit: I think it'd be a little clearer to move these to the top of the function and define maxSignatureLength next to them.

Ok.

> > WebCore/platform/image-decoders/ImageDecoder.cpp:79
> > +    DEFINE_STATIC_LOCAL(Vector<char>, ICOSignature2, ("\x00\x00\x02\x00", 4));
> 
> Nit: I suggest "CURSignature" instead for clarity.

Ah, good idea.

> > WebCore/platform/image-decoders/ImageDecoder.cpp:81
> > +    if (matcheSignature(GIFSignature1, contents) || matcheSignature(GIFSignature2, contents))
> 
> Won't compile: matcheSignature -> matchesSignature

I guess this file isn't used in the Mac build.

> > WebCore/platform/image-decoders/ImageDecoder.cpp:93
> >      // ICOs always begin with a 2-byte 0 followed by a 2-byte 1.
> 
> Nit: These comments aren't really needed.

Yeah, especially once I rename the signatures as you suggest.
Comment 8 Adam Barth 2010-10-14 14:35:30 PDT
Created attachment 70779 [details]
Patch
Comment 9 WebKit Review Bot 2010-10-14 15:08:29 PDT
Attachment 70779 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4467019
Comment 10 WebKit Review Bot 2010-10-14 15:08:36 PDT
Attachment 70779 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4419034
Comment 11 Adam Barth 2010-10-14 16:24:46 PDT
Comment on attachment 70779 [details]
Patch

I'll make it compile before landing.
Comment 12 Eric Seidel (no email) 2010-10-14 16:51:00 PDT
I looked into creating a test for this.  It turns out that Drag and drop knows the extension of the image.  So if we dragged the image off screen we could get its extension.

JavaScript however has no way to get the extension (or mimetype) of a displayed image as far as I can tell.
Comment 13 Eric Seidel (no email) 2010-10-14 16:52:10 PDT
Created attachment 70799 [details]
the test case I was playing around with (for posterity)
Comment 14 Eric Seidel (no email) 2010-11-02 02:59:32 PDT
Comment on attachment 70779 [details]
Patch

I really wish we had better testing for our image decoders.
Comment 15 Adam Barth 2010-11-02 16:42:10 PDT
Created attachment 72767 [details]
merge to TOT
Comment 16 Adam Barth 2010-11-02 19:09:32 PDT
Created attachment 72784 [details]
merge to TOT
Comment 17 Pascal Massimino 2010-11-03 18:01:54 PDT
(In reply to comment #16)
> Created an attachment (id=72784) [details]
> merge to TOT

shouldn't the WEBSignature be: "RIFF????WEBPVP" (14 symbols, with four '?'s) ?
Comment 18 Adam Barth 2010-11-03 18:05:46 PDT
> shouldn't the WEBSignature be: "RIFF????WEBPVP" (14 symbols, with four '?'s) ?

That probably explains why the WebP test is failing.  :)
Comment 19 Adam Barth 2011-05-23 10:28:46 PDT
Created attachment 94439 [details]
Patch
Comment 20 Eric Seidel (no email) 2011-05-23 10:32:32 PDT
Comment on attachment 94439 [details]
Patch

OK.  Seems we could test this by having a green square PNG and modifying its signature, or similarly a GIF.  And make sure that it loads or doesn't load.
Comment 21 Adam Barth 2011-05-23 10:37:29 PDT
Possibly, unless something else in the network stack obscures that difference.  I can try writing such a test.
Comment 22 Adam Barth 2011-05-23 10:52:02 PDT
Created attachment 94443 [details]
Patch
Comment 23 Eric Seidel (no email) 2011-05-23 10:52:58 PDT
Comment on attachment 94443 [details]
Patch

OK.
Comment 24 WebKit Commit Bot 2011-05-23 12:48:06 PDT
Comment on attachment 94443 [details]
Patch

Clearing flags on attachment: 94443

Committed r87090: <http://trac.webkit.org/changeset/87090>
Comment 25 WebKit Commit Bot 2011-05-23 12:48:13 PDT
All reviewed patches have been landed.  Closing bug.