Bug 110620 – Webkit unable to show gifs with applcation extension string shorter than 11 bytes.

• Component: Images
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

From http://code.google.com/p/chromium/issues/detail?id=177561

Webkit doesn't able to decode and show images where application extension length is below 11 bytes.
By the specification length of the application extension has to be exactly 11 byte. Unfortunately a lot of apps just use this extension as regular passcal string with length above and below 11 characters. Most of image viewers and decoders are able to decode this gifs, so webkit should be able also.

[Viatcheslav Ostapenko]

Created attachment 189787 [details]
GIF image with short application extension string.

[Viatcheslav Ostapenko]

Created attachment 189794 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189794 [details]
Patch

Attachment 189794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16711204

New failing tests:
platform/chromium/virtual/deferred/fast/images/gif-short-app-extension-string.html
fast/images/gif-short-app-extension-string.html

[Viatcheslav Ostapenko]

Created attachment 189811 [details]
Patch

[Peter Kasting]

No, we can't do this.

Did you read the comment about this above the code you're changing?  See http://trac.webkit.org/changeset/117333 in particular (and http://trac.webkit.org/changeset/117373 as a followup) for more info.

We correctly decode cases with values above 11.  What cases on the web rely on correct decoding below 11?  Note that Mozilla should refuse to decode these either, for the same reason.

I think this is WONTFIX.  These GIFs are malformed.

[Viatcheslav Ostapenko]

(In reply to comment #5)
> No, we can't do this.
> 
> Did you read the comment about this above the code you're changing?  See http://trac.webkit.org/changeset/117333 in particular (and http://trac.webkit.org/changeset/117373 as a followup) for more info.

It is definitely can't cause out of buffer read because of cycle len >= m_bytesToConsume condition. Otherwise those bytes are just skipped and not used anywhere.
I looked through the source of many apps and they use spec. length value for other extensions, but of app. extension uses length value as it is.

> We correctly decode cases with values above 11.  What cases on the web rely on correct decoding below 11?  Note that Mozilla should refuse to decode these either, for the same reason.

But IE and Opera decode it without problems, and also most of image processing apps.

[Peter Kasting]

(In reply to comment #6)
> > We correctly decode cases with values above 11.  What cases on the web rely on correct decoding below 11?  Note that Mozilla should refuse to decode these either, for the same reason.
> 
> But IE and Opera decode it without problems, and also most of image processing apps.

You didn't answer my question.  Where on the web relies on this?  I'm less concerned about theoretical problems that a GIF could have than actual problems real websites run into.  And given that Gecko and WebKit have always assumed at least 11 bytes here, I worry about real sites relying on this -- i.e. having broken GIFs which claim <11 bytes for this extension but then actually put 11 bytes in it.  How do you know no such sites exist?

If we knew for sure that we wanted to make this change and it wouldn't cause any bustage, I think we could do it, but I think we'd want to change the handling of the plain text extension (case 0x01) as well, and rewrite the comment to be clear that only the GIFControlExtension handler expects at least 4 bytes.

Furthermore, your strncmp() calls are wrong.  For cases where m_count is greater than 11, but the first 11 bytes match, we'll wrongly go to the GIFConsumeBlock state instead of the GIFNetscapeExtensionBlock state.  And for cases where m_count is less than 11 but all m_count characters match the beginning of one of the two compared identifiers, we'll jump to the GIFNetscapeExtensionBlock state, but we should probably go to the GIFConsumeBlock state instead.

[Laszlo Gombos]

> You didn't answer my question.  Where on the web relies on this?

It seems that Amazon Cloud Reader is serving these images as it is reported in the original chromium bug report that is referenced in the first line of the webkit bug description.

As long as some of the "other" image decoders and browser behave differently, I think this issue will be raised over and over again, so regardless of the final decision I think we should have a LayoutTest with an image that specifically test for this condition so that we can point back to this decision.

[Peter Kasting]

(In reply to comment #8)
> > You didn't answer my question.  Where on the web relies on this?
> 
> It seems that Amazon Cloud Reader is serving these images as it is reported in the original chromium bug report that is referenced in the first line of the webkit bug description.

Sigh, I missed that line entirely on every single readthrough.

Fair enough, then see my implementation comments in comment 7.

[Viatcheslav Ostapenko]

(In reply to comment #7)
> (In reply to comment #6)
> > > We correctly decode cases with values above 11.  What cases on the web rely on correct decoding below 11?  Note that Mozilla should refuse to decode these either, for the same reason.
> > 
> > But IE and Opera decode it without problems, and also most of image processing apps.
> 
> You didn't answer my question.  Where on the web relies on this?  

Sorry, I missed it. Laszlo already have answered.

> I'm less concerned about theoretical problems that a GIF could have than actual problems real websites run into.  And given that Gecko and WebKit have always assumed at least 11 bytes here, I worry about real sites relying on this -- i.e. having broken GIFs which claim <11 bytes for this extension but then actually put 11 bytes in it.  How do you know no such sites exist?

ImageMagick actually 1st tries to match all commonly used strings like NETSCAPE2.0, ICCRGBG1012, MGK8BIM0000, MGKIPTC0000 and only after that uses whatever string it gets in the block. 
For this file "identify" tool decodes this extension like:

Profile-gif:MBPW: 256 bytes 
 
> If we knew for sure that we wanted to make this change and it wouldn't cause any bustage, I think we could do it, but I think we'd want to change the handling of the plain text extension (case 0x01) as well, and rewrite the comment to be clear that only the GIFControlExtension handler expects at least 4 bytes.

Actually, the text extension header has data structure with predefined fields in a header block, so I would leave check that it has at least 12 bytes.

> Furthermore, your strncmp() calls are wrong.  For cases where m_count is greater than 11, but the first 11 bytes match, we'll wrongly go to the GIFConsumeBlock state instead of the GIFNetscapeExtensionBlock state.  And for cases where m_count is less than 11 but all m_count characters match the beginning of one of the two compared identifiers, we'll jump to the GIFNetscapeExtensionBlock state, but we should probably go to the GIFConsumeBlock state instead.

Thanks, I fixed this.

[Viatcheslav Ostapenko]

Created attachment 189914 [details]
Patch

[Viatcheslav Ostapenko]

Created attachment 189915 [details]
Patch

[Peter Kasting]

(In reply to comment #10)
> Actually, the text extension header has data structure with predefined fields in a header block, so I would leave check that it has at least 12 bytes.

But we don't care what those are.  We don't actually try to read any of that data.

Sure, not having a 12-byte length value for this extension would mean the GIF is invalid.  But a non-11-byte application extension ID is invalid too.  I don't see why we'd want to enforce one and not the other.

[Peter Kasting]

Comment on attachment 189915 [details]
Patch

Rather than add more detail to the existing comment, I'd rewrite the comment entirely to be multiple distinct comments in the different case handlers explaining for each case why we handle it the way we do.

[Viatcheslav Ostapenko]

Created attachment 189961 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #14)
> (From update of attachment 189915 [details])
> Rather than add more detail to the existing comment, I'd rewrite the comment entirely to be multiple distinct comments in the different case handlers explaining for each case why we handle it the way we do.

Fixed.

[Peter Kasting]

Comment on attachment 189961 [details]
Patch

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

Seems OK to me.  Get a reviewer to give the formal r+.

Please also file this against Gecko at bugzilla.mozilla.org , since GIFImageReader.cpp is derived from their code, and they patched this portion of their code to act like ours the last time we touched it.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:554
> +                // The GIF spec mandates length of the GIF Control Extension to be 4 bytes.

Nit: This comment has a few issues; how about:

The GIF spec mandates that the GIFControlExtension header block length is 4 bytes, and the parser for this block reads 4 bytes, so we must enforce that the buffer contains at least this many bytes.  If the GIF specifies a different length, we allow that, so long as it's larger; the additional data will simply be ignored.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:565
> +            // GIF spec. also dictates fixed length header block for the following extension,

Nit: How about:

The GIF spec also specifies the lengths of the following two extensions' headers (as 12 and 11 bytes, respectively).  Because we ignore the plain text extension entirely and sanity-check the actual length of the application extension header before reading it, we allow GIFs to deviate from these values in either direction.  This is important for real-world compatibility, as GIFs in the wild exist with application extension headers that are both shorter and longer than 11 bytes.

[Viatcheslav Ostapenko]

Created attachment 189993 [details]
Patch

[Viatcheslav Ostapenko]

(In reply to comment #17)
> (From update of attachment 189961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189961&action=review
> 
> Seems OK to me.  Get a reviewer to give the formal r+.
> 
> Please also file this against Gecko at bugzilla.mozilla.org , since GIFImageReader.cpp is derived from their code, and they patched this portion of their code to act like ours the last time we touched it.

https://bugzilla.mozilla.org/show_bug.cgi?id=844684

[Laszlo Gombos]

Comment on attachment 189993 [details]
Patch

LGTM. 

The same patch landed in Mozilla as already. Thanks Peter for your help !

[WebKit Review Bot]

Comment on attachment 189993 [details]
Patch

Rejecting attachment 189993 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 189993, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp.rej
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/images/gif-short-app-extension-string-expected.txt
patching file LayoutTests/fast/images/gif-short-app-extension-string.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Laszlo Gombos']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/16815087

[Viatcheslav Ostapenko]

Created attachment 190661 [details]
Patch

[WebKit Review Bot]

Attachment 190661 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.png', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.txt', u'LayoutTests/fast/images/gif-short-app-extension-string.html', u'LayoutTests/fast/images/resources/short-app-extension-string.gif', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp']" exit_code: 1
LayoutTests/fast/images/gif-short-app-extension-string-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Created attachment 190670 [details]
Patch

[WebKit Review Bot]

Attachment 190670 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.png', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.txt', u'LayoutTests/fast/images/gif-short-app-extension-string.html', u'LayoutTests/fast/images/resources/short-app-extension-string.gif', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp']" exit_code: 1
LayoutTests/fast/images/gif-short-app-extension-string-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Created attachment 191403 [details]
Patch

[WebKit Review Bot]

Attachment 191403 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.png', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.txt', u'LayoutTests/fast/images/gif-short-app-extension-string.html', u'LayoutTests/fast/images/resources/short-app-extension-string.gif', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp']" exit_code: 1
LayoutTests/fast/images/gif-short-app-extension-string-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Created attachment 192504 [details]
Patch

[WebKit Review Bot]

Attachment 192504 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.png', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.txt', u'LayoutTests/fast/images/gif-short-app-extension-string.html', u'LayoutTests/fast/images/resources/short-app-extension-string.gif', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp']" exit_code: 1
LayoutTests/fast/images/gif-short-app-extension-string-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Created attachment 192647 [details]
Patch

[WebKit Review Bot]

Attachment 192647 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.png', u'LayoutTests/fast/images/gif-short-app-extension-string-expected.txt', u'LayoutTests/fast/images/gif-short-app-extension-string.html', u'LayoutTests/fast/images/resources/short-app-extension-string.gif', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp']" exit_code: 1
LayoutTests/fast/images/gif-short-app-extension-string-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Viatcheslav Ostapenko]

Created attachment 192657 [details]
Patch

[Laszlo Gombos]

Comment on attachment 192657 [details]
Patch

r=me (again).

[WebKit Review Bot]

Comment on attachment 192657 [details]
Patch

Clearing flags on attachment: 192657

Committed r145569: <http://trac.webkit.org/changeset/145569>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.