image/gif: handle an extra data sub-block byte.

This changes the decoder's behaviour when there is stray/extra data
found after an image is decompressed (e.g., data sub-blocks after an LZW
End of Information Code). Instead of raising an error, we silently skip
over such data until we find the end of the image data marked by a Block
Terminator. We skip at most one byte as sample problem GIFs exhibit this
property.

GIFs should not have and do not need such stray data (though the
specification is arguably ambiguous). However GIFs with such properties
have been seen in the wild.

Fixes #16146

Change-Id: Ie7e69052bab5256b4834992304e6ca58e93c1879
Reviewed-on: https://go-review.googlesource.com/37258
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/image/gif/reader.go

@@ -231,8 +231,8 @@ func (d *decoder) decode(r io.Reader, configOnly bool) error {
 				}
 				return errNotEnough
 			}
-			// Both lzwr and br should be exhausted. Reading from them should
-			// yield (0, io.EOF).
+			// In theory, both lzwr and br should be exhausted. Reading from them
+			// should yield (0, io.EOF).
 			//
 			// The spec (Appendix F - Compression), says that "An End of
 			// Information code... must be the last code output by the encoder
@@ -248,11 +248,21 @@ func (d *decoder) decode(r io.Reader, configOnly bool) error {
 				}
 				return errTooMuch
 			}
-			if n, err := br.Read(d.tmp[:1]); n != 0 || err != io.EOF {
+
+			// In practice, some GIFs have an extra byte in the data sub-block
+			// stream, which we ignore. See https://golang.org/issue/16146.
+			for nExtraBytes := 0; ; {
+				n, err := br.Read(d.tmp[:2])
+				nExtraBytes += n
+				if nExtraBytes > 1 {
+					return errTooMuch
+				}
+				if err == io.EOF {
+					break
+				}
 				if err != nil {
 					return fmt.Errorf("gif: reading image data: %v", err)
 				}
-				return errTooMuch
 			}
 
 			// Check that the color indexes are inside the palette.

src/image/gif/reader_test.go

@@ -37,16 +37,35 @@ func lzwEncode(in []byte) []byte {
 }
 
 func TestDecode(t *testing.T) {
+	// extra contains superfluous bytes to inject into the GIF, either at the end
+	// of an existing data sub-block (past the LZW End of Information code) or in
+	// a separate data sub-block. The 0x02 values are arbitrary.
+	const extra = "\x02\x02\x02\x02"
+
 	testCases := []struct {
-		nPix    int  // The number of pixels in the image data.
-		extra   bool // Whether to write an extra block after the LZW-encoded data.
-		wantErr error
+		nPix int // The number of pixels in the image data.
+		// If non-zero, write this many extra bytes inside the data sub-block
+		// containing the LZW end code.
+		extraExisting int
+		// If non-zero, write an extra block of this many bytes.
+		extraSeparate int
+		wantErr       error
 	}{
-		{0, false, errNotEnough},
-		{1, false, errNotEnough},
-		{2, false, nil},
-		{2, true, errTooMuch},
-		{3, false, errTooMuch},
+		{0, 0, 0, errNotEnough},
+		{1, 0, 0, errNotEnough},
+		{2, 0, 0, nil},
+		// An extra data sub-block after the compressed section with 1 byte which we
+		// silently skip.
+		{2, 0, 1, nil},
+		// An extra data sub-block after the compressed section with 2 bytes. In
+		// this case we complain that there is too much data.
+		{2, 0, 2, errTooMuch},
+		// Too much pixel data.
+		{3, 0, 0, errTooMuch},
+		// An extra byte after LZW data, but inside the same data sub-block.
+		{2, 1, 0, nil},
+		// Two extra bytes after LZW data, but inside the same data sub-block.
+		{2, 2, 0, nil},
 	}
 	for _, tc := range testCases {
 		b := &bytes.Buffer{}
@@ -59,22 +78,35 @@ func TestDecode(t *testing.T) {
 		b.WriteString("\x2c\x00\x00\x00\x00\x02\x00\x01\x00\x00\x02")
 		if tc.nPix > 0 {
 			enc := lzwEncode(make([]byte, tc.nPix))
-			if len(enc) > 0xff {
-				t.Errorf("nPix=%d, extra=%t: compressed length %d is too large", tc.nPix, tc.extra, len(enc))
+			if len(enc)+tc.extraExisting > 0xff {
+				t.Errorf("nPix=%d, extraExisting=%d, extraSeparate=%d: compressed length %d is too large",
+					tc.nPix, tc.extraExisting, tc.extraSeparate, len(enc))
 				continue
 			}
-			b.WriteByte(byte(len(enc)))
+
+			// Write the size of the data sub-block containing the LZW data.
+			b.WriteByte(byte(len(enc) + tc.extraExisting))
+
+			// Write the LZW data.
 			b.Write(enc)
+
+			// Write extra bytes inside the same data sub-block where LZW data
+			// ended. Each arbitrarily 0x02.
+			b.WriteString(extra[:tc.extraExisting])
 		}
-		if tc.extra {
-			b.WriteString("\x01\x02") // A 1-byte payload with an 0x02 byte.
+
+		if tc.extraSeparate > 0 {
+			// Data sub-block size. This indicates how many extra bytes follow.
+			b.WriteByte(byte(tc.extraSeparate))
+			b.WriteString(extra[:tc.extraSeparate])
 		}
 		b.WriteByte(0x00) // An empty block signifies the end of the image data.
 		b.WriteString(trailerStr)
 
 		got, err := Decode(b)
 		if err != tc.wantErr {
-			t.Errorf("nPix=%d, extra=%t\ngot  %v\nwant %v", tc.nPix, tc.extra, err, tc.wantErr)
+			t.Errorf("nPix=%d, extraExisting=%d, extraSeparate=%d\ngot  %v\nwant %v",
+				tc.nPix, tc.extraExisting, tc.extraSeparate, err, tc.wantErr)
 		}
 
 		if tc.wantErr != nil {
@@ -90,7 +122,8 @@ func TestDecode(t *testing.T) {
 			},
 		}
 		if !reflect.DeepEqual(got, want) {
-			t.Errorf("nPix=%d, extra=%t\ngot  %v\nwant %v", tc.nPix, tc.extra, got, want)
+			t.Errorf("nPix=%d, extraExisting=%d, extraSeparate=%d\ngot  %v\nwant %v",
+				tc.nPix, tc.extraExisting, tc.extraSeparate, got, want)
 		}
 	}
 }