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 123166 ca131cc6fdac33a7ebcbcfd6e1f69a52ba24e982
parent 123165 62f32eb94356870e47af4490fd1a0bc845d6812c
child 123167 17d25f2c3c3899e538c026638e2e14fac1ab0ff4
push id1390
push userMs2ger@gmail.com
push dateThu, 28 Feb 2013 17:40:16 +0000
treeherderfx-team@c65d59d33aa8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjoe
bugs844684
milestone22.0a1
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 0000000000000000000000000000000000000000..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>