Bug 47683 – Update ImageDecoder::create to match sniffing spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Update ImageDecoder::create to match sniffing spec

[Adam Barth]

Created attachment 70769 [details]
Patch

[WebKit Review Bot]

Attachment 70769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4436039

[Peter Kasting]

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.

[Eric Seidel (no email)]

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?

[Peter Kasting]

(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.

[WebKit Review Bot]

Attachment 70769 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4466044

[Adam Barth]

> 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.

[Adam Barth]

Created attachment 70779 [details]
Patch

[WebKit Review Bot]

Attachment 70779 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4467019

[WebKit Review Bot]

Attachment 70779 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4419034

[Adam Barth]

Comment on attachment 70779 [details]
Patch

I'll make it compile before landing.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 70799 [details]
the test case I was playing around with (for posterity)

[Eric Seidel (no email)]

Comment on attachment 70779 [details]
Patch

I really wish we had better testing for our image decoders.

[Adam Barth]

Created attachment 72767 [details]
merge to TOT

[Adam Barth]

Created attachment 72784 [details]
merge to TOT

[Pascal Massimino]

(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) ?

[Adam Barth]

> shouldn't the WEBSignature be: "RIFF????WEBPVP" (14 symbols, with four '?'s) ?

That probably explains why the WebP test is failing.  :)

[Adam Barth]

Created attachment 94439 [details]
Patch

[Eric Seidel (no email)]

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.

[Adam Barth]

Possibly, unless something else in the network stack obscures that difference.  I can try writing such a test.

[Adam Barth]

Created attachment 94443 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 94443 [details]
Patch

OK.

[WebKit Commit Bot]

Comment on attachment 94443 [details]
Patch

Clearing flags on attachment: 94443

Committed r87090: <http://trac.webkit.org/changeset/87090>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.