Bug 110620 - Webkit unable to show gifs with applcation extension string shorter than 11 bytes.
Summary: Webkit unable to show gifs with applcation extension string shorter than 11 b...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Viatcheslav Ostapenko
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-22 09:57 PST by Viatcheslav Ostapenko
Modified: 2013-03-12 11:29 PDT (History)
4 users (show)

See Also:


Attachments
GIF image with short application extension string. (1.45 KB, image/gif)
2013-02-22 10:01 PST, Viatcheslav Ostapenko
no flags Details
Patch (6.01 KB, patch)
2013-02-22 10:52 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2013-02-22 13:14 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (8.95 KB, patch)
2013-02-23 01:43 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (8.95 KB, patch)
2013-02-23 01:48 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (10.18 KB, patch)
2013-02-24 01:17 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (10.08 KB, patch)
2013-02-24 19:32 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2013-02-27 23:08 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2013-02-28 00:32 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (13.87 KB, patch)
2013-03-04 22:27 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (10.35 KB, patch)
2013-03-11 10:51 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2013-03-11 23:07 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2013-03-12 00:41 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2013-02-22 09:57:48 PST
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.
Comment 1 Viatcheslav Ostapenko 2013-02-22 10:01:34 PST
Created attachment 189787 [details]
GIF image with short application extension string.
Comment 2 Viatcheslav Ostapenko 2013-02-22 10:52:18 PST
Created attachment 189794 [details]
Patch
Comment 3 WebKit Review Bot 2013-02-22 13:04:56 PST
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
Comment 4 Viatcheslav Ostapenko 2013-02-22 13:14:09 PST
Created attachment 189811 [details]
Patch
Comment 5 Peter Kasting 2013-02-22 13:27:04 PST
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.
Comment 6 Viatcheslav Ostapenko 2013-02-22 14:42:45 PST
(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.
Comment 7 Peter Kasting 2013-02-22 16:24:15 PST
(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.
Comment 8 Laszlo Gombos 2013-02-22 19:29:19 PST
> 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.
Comment 9 Peter Kasting 2013-02-22 20:15:59 PST
(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.
Comment 10 Viatcheslav Ostapenko 2013-02-23 01:36:38 PST
(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.
Comment 11 Viatcheslav Ostapenko 2013-02-23 01:43:04 PST
Created attachment 189914 [details]
Patch
Comment 12 Viatcheslav Ostapenko 2013-02-23 01:48:16 PST
Created attachment 189915 [details]
Patch
Comment 13 Peter Kasting 2013-02-23 10:51:11 PST
(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.
Comment 14 Peter Kasting 2013-02-23 10:53:02 PST
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.
Comment 15 Viatcheslav Ostapenko 2013-02-24 01:17:06 PST
Created attachment 189961 [details]
Patch
Comment 16 Viatcheslav Ostapenko 2013-02-24 01:17:55 PST
(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.
Comment 17 Peter Kasting 2013-02-24 16:21:25 PST
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.
Comment 18 Viatcheslav Ostapenko 2013-02-24 19:32:26 PST
Created attachment 189993 [details]
Patch
Comment 19 Viatcheslav Ostapenko 2013-02-24 19:33:47 PST
(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
Comment 20 Laszlo Gombos 2013-02-27 14:51:11 PST
Comment on attachment 189993 [details]
Patch

LGTM. 

The same patch landed in Mozilla as already. Thanks Peter for your help !
Comment 21 WebKit Review Bot 2013-02-27 20:08:51 PST
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
Comment 22 Viatcheslav Ostapenko 2013-02-27 23:08:17 PST
Created attachment 190661 [details]
Patch
Comment 23 WebKit Review Bot 2013-02-27 23:10:55 PST
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.
Comment 24 Viatcheslav Ostapenko 2013-02-28 00:32:18 PST
Created attachment 190670 [details]
Patch
Comment 25 WebKit Review Bot 2013-02-28 00:38:10 PST
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.
Comment 26 Viatcheslav Ostapenko 2013-03-04 22:27:47 PST
Created attachment 191403 [details]
Patch
Comment 27 WebKit Review Bot 2013-03-04 22:31:55 PST
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.
Comment 28 Viatcheslav Ostapenko 2013-03-11 10:51:29 PDT
Created attachment 192504 [details]
Patch
Comment 29 WebKit Review Bot 2013-03-11 10:54:56 PDT
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.
Comment 30 Viatcheslav Ostapenko 2013-03-11 23:07:02 PDT
Created attachment 192647 [details]
Patch
Comment 31 WebKit Review Bot 2013-03-11 23:09:57 PDT
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.
Comment 32 Viatcheslav Ostapenko 2013-03-12 00:41:18 PDT
Created attachment 192657 [details]
Patch
Comment 33 Laszlo Gombos 2013-03-12 10:53:32 PDT
Comment on attachment 192657 [details]
Patch

r=me (again).
Comment 34 WebKit Review Bot 2013-03-12 11:29:24 PDT
Comment on attachment 192657 [details]
Patch

Clearing flags on attachment: 192657

Committed r145569: <http://trac.webkit.org/changeset/145569>
Comment 35 WebKit Review Bot 2013-03-12 11:29:29 PDT
All reviewed patches have been landed.  Closing bug.