Bug 844684 - Decode GIFs that include an application extension string shorter than 11 bytes. r=joe
authorJosh Matthews <josh@joshmatthews.net>
Wed, 27 Feb 2013 00:24:18 -0500
changeset 123116 ca131cc6fdac33a7ebcbcfd6e1f69a52ba24e982
parent 123115 62f32eb94356870e47af4490fd1a0bc845d6812c
child 123117 17d25f2c3c3899e538c026638e2e14fac1ab0ff4
push id24372
push useremorley@mozilla.com
push dateWed, 27 Feb 2013 13:22:59 +0000
treeherdermozilla-central@0a91da5f5eab [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjoe
bugs844684
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 844684 - Decode GIFs that include an application extension string shorter than 11 bytes. r=joe
image/decoders/nsGIFDecoder2.cpp
image/test/mochitest/Makefile.in
image/test/mochitest/short_header.gif
image/test/mochitest/test_short_gif_header.html
--- a/image/decoders/nsGIFDecoder2.cpp
+++ b/image/decoders/nsGIFDecoder2.cpp
@@ -757,49 +757,40 @@ nsGIFDecoder2::WriteInternal(const char 
           } else {
             /* No images decoded, there is nothing to display. */
             mGIFStruct.state = gif_error;
           }
       }
       break;
 
     case gif_extension:
-      // Comment taken directly from WebKit's GIFImageReader.cpp.
-      //
-      // The GIF spec mandates lengths for three of the extensions below.
-      // However, it's possible for GIFs in the wild to deviate. For example,
-      // some GIFs that embed ICC color profiles using gif_application_extension
-      // violate the spec and treat this extension block like a sort of
-      // "extension + data" block, giving a size greater than 11 and filling the
-      // remaining bytes with data (then following with more data blocks as
-      // needed), instead of placing a true data block just after the 11 byte
-      // extension block.
-      //
-      // Accordingly, if the specified length is larger than the required value,
-      // we use it. If it's smaller, then we enforce the spec value, because the
-      // parsers for these extensions expect to have the specified number of
-      // bytes available, and if we don't ensure that, they could read off the
-      // end of the heap buffer. (In this case, it's likely the GIF is corrupt
-      // and we'll soon fail to decode anyway.)
       mGIFStruct.bytes_to_consume = q[1];
       if (mGIFStruct.bytes_to_consume) {
         switch (*q) {
         case GIF_GRAPHIC_CONTROL_LABEL:
+          // 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.
           mGIFStruct.state = gif_control_extension;
           mGIFStruct.bytes_to_consume = std::max(mGIFStruct.bytes_to_consume, 4u);
           break;
 
+        // 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.
         case GIF_APPLICATION_EXTENSION_LABEL:
           mGIFStruct.state = gif_application_extension;
-          mGIFStruct.bytes_to_consume = std::max(mGIFStruct.bytes_to_consume, 11u);
           break;
 
         case GIF_PLAIN_TEXT_LABEL:
           mGIFStruct.state = gif_skip_block;
-          mGIFStruct.bytes_to_consume = std::max(mGIFStruct.bytes_to_consume, 12u);
           break;
 
         case GIF_COMMENT_LABEL:
           mGIFStruct.state = gif_consume_comment;
           break;
 
         default:
           mGIFStruct.state = gif_skip_block;
@@ -840,18 +831,19 @@ nsGIFDecoder2::WriteInternal(const char 
       break;
 
     case gif_consume_comment:
       GETN(1, gif_comment_extension);
       break;
 
     case gif_application_extension:
       /* Check for netscape application extension */
-      if (!strncmp((char*)q, "NETSCAPE2.0", 11) ||
-        !strncmp((char*)q, "ANIMEXTS1.0", 11))
+      if (mGIFStruct.bytes_to_consume == 11 &&
+          (!strncmp((char*)q, "NETSCAPE2.0", 11) ||
+           !strncmp((char*)q, "ANIMEXTS1.0", 11)))
         GETN(1, gif_netscape_extension_block);
       else
         GETN(1, gif_consume_block);
       break;
 
     /* Netscape-specific GIF extension: animation looping */
     case gif_netscape_extension_block:
       if (*q)
--- a/image/test/mochitest/Makefile.in
+++ b/image/test/mochitest/Makefile.in
@@ -62,16 +62,18 @@ MOCHITEST_FILES =   imgutils.js \
                 bad.jpg \
                 rillybad.jpg \
                 test_bug767779.html \
                 bug767779.sjs \
                 animated-gif_trailing-garbage.gif \
 		test_error_events.html \
 		error-early.png \
 		test_drawDiscardedImage.html \
+		short_header.gif \
+		test_short_gif_header.html \
                 $(NULL)
 
 # Tests disabled due to intermittent orange
 # test_bug435296.html disabled - See bug 578591
 # test_bug478398.html disabled - See bug 579139
 
 MOCHITEST_CHROME_FILES = imgutils.js \
                 animationPolling.js \
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..70af95ac6dd80eab9bc077499e60f095edffb719
GIT binary patch
literal 1488
zc${6-`9Bkm1IO{XawH8Y3&lvT<c^5BW<=(gqmX0CnJZf*WymorWo~lLedNB0By)%4
zoO{k%ax3if?eX}2KA*>D&mUg@!TYVJ4^>uiaG(PL_NajWp`)Xtr>AFNU|?iqWMX1E
zapD9sGcyYd3o9!t8ynlnlPB5P*-xE1#lgXG`t)fa5Xi~N$;HLR&CPx0%o!dYp0j7q
zo;!DrmzNg=0-Zm9o{x`@pPyeqK;XiK3l}e56ciK`5)uM~!NS7AA|fJ}E?p886}^1<
zvY42dxVX54goLD|q?DA@l`B`IrKPW4y(%LkBP%N_CnqN_FR!4Wps1**q@;B1+BIcm
zWfc{b>({TVs;a7~sol77LtR~6Lqh`sfoN)KYH4X{YisN1=;-R|LZMJSJv|r<rmwGW
zU|?|b=1oIGLn9-jTeof*8ymyna1#>~Q&UqjGc$8@a|8lmVPRouX=!C;Wo>PJ`}XZS
zckbBO*x1_I+S%FNy?fW*-rm8%!O_vt$;k<cMBcl1&)M1e{{8zdE-tRFu5NB_?(Xg$
z9v+^ao(~>8@bdEV_V)Ji@$vQb_4D(4`0$~>zds6vLZi`-9zA;e`0<k`PXYo0o<4mV
z7#R5M*|X=*p9cj6VKA8B;NXyukkHW37cX9fg@uKOhet$2L`FtpvDm1nC>##=^5x4{
zuU<t*N56jk`pug+F)=ZCJU%uyHZCqMK0ZDnAt5m_F)1l2IXO8cB_%aA_3hiYX=!Qc
z>FF668JU@xSy@@x+1UgFAtxs%H#avgFE2kozo4L?u&}VGsHnKOxTK_{w6yfyyLV+}
zW##4N6%`egm6cUhRn^tiH8nN0wY7D1b@lc24Gj&Ajg3uBP0h{CEiEnY-@k8dZEb67
z`|#mIdwcuGj~_caIzD~+)Y;idBoe#2y1Ki&dwP0$dwcu(`uh9(2L=W{fBrl;I5;#k
zG(0@~<;$0mk&&-ozmATMj*X3tkB?7GOiWHrPEAdbNTliM>6w|C+1c5-xw-lI`Gtjr
z#l=N3nY^^Lw7k5$va+(ey1KTuw!Xgp?c2BS-@j8RlpjBSY;0`&{P}Zpb8~BJYkPZp
zXJ==3cXw}ZZ-0OP;NalbuV24^|2{lCJUTi$K0c;WsX_;ge>MORqB0t3!7YQ9sQ))g
zYm8P4t#Mitv~p-o(h8cQB}a=yYx-Yve*^u;46RvOfPYB+$)9%sup9%Zto*RDjJ6OK
zZV4M=S!R1UP}m?9R-V-n#VhYNMJ&(mj26_!^6OU+y6|FVg*IIkIXww7PTi^cmAQQ>
z%6@B8U6pwQ>5ve0B$OZ~p9#ZD*fs!LOK~JQL_MB!kR1$`lO=Um7eVsa8WoWi9TEtf
z&eYK)6Qp>w);loRNO{3S8;s)Nvd-UGAFc~xK=+CS6LBDVQc}Idyx3qC=Vt#AI?u(S
znnH=#Zv=X2131vSaLKwVS1iDsRn0vY+iIvs5;)3Z+ivaZl`*D33E0WdG|+_IiuC&G
z0+BD7(<gju5z|OPYAGDfAko9)zQ?1UP@W-{5Q1>AAtPk4g_+a`NpETn!1QK0B2pUc
zy=xek8=^MQ70$bJ<T$whYRTR-h9a~ZtqR~DnSg8;K&}1#%5&mT?y}5jcJyGK3{6BL
zW;TL|FtM_YbVW^Tg!1?^1Il=qS`i#rnGqZnD4U`ctqUKsWAnEH0(7`oMIvFM1&c9;
nN~?=_I8>Y*Yi{96j<a$uAjjLFR>=wWVd6`P$oK+N2tf5Ol>8G0
new file mode 100644
--- /dev/null
+++ b/image/test/mochitest/test_short_gif_header.html
@@ -0,0 +1,35 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=844684
+-->
+<head>
+  <title>Test for Bug 844684</title>
+  <script type="application/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=844684">Mozilla Bug 844684</a>
+<div id="content">
+<img id="testcontent" onload="success()" onerror="failure()">
+</div>
+<pre id="test">
+<script type="application/javascript">
+SimpleTest.waitForExplicitFinish();
+
+document.getElementById('testcontent').src = "short_header.gif";
+
+function success() {
+  ok(true, "Image loaded.");
+  SimpleTest.finish();
+}
+
+function failure() {
+  ok(false, "Image didn't load.");
+  SimpleTest.finish();
+}
+</script>
+</pre>
+</body>
+</html>