From 60b7d27c8282f191a89895e9a925b12d78d33411 Mon Sep 17 00:00:00 2001
From: Nigel Tao <nigeltao@golang.org>
Date: Tue, 14 Jul 2015 15:46:18 +1000
Subject: [PATCH] image/jpeg: don't unread a byte if we've already taken bits
 from it.

This rolls back most of golang.org/cl/8841, aka 2f98bac310, and makes a
different fix. It keeps the TestTruncatedSOSDataDoesntPanic test
introduced by that other CL, which obviously still passes after this CL.

Fixes #11650, a regression (introduced by cl/8841) from Go 1.4.

The original cl/8841 changed the image/jpeg not to panic on an input
given in #10387. We still do not panic on that input, after this CL.

I have a corpus of over 160,000 JPEG images, a sample of a web crawl.
The image/jpeg code ran happily over that whole corpus both before and
after this CL, although that corpus clearly didn't catch the regression
in the first place.

This code was otherwise tested manually. I don't think that it's trivial
to synthesize a JPEG input that happens to run out of Huffman data at
just the right place. The test image attached to #11650 obviously has
that property, but I don't think we can simply add that test image to
the repository: it's 227KiB, and I don't know its copyright status.

I also looked back over the issue tracker for problematic JPEGs that
people have filed. The Go code, after this CL, is still happy on these
files in my directory:
issue2362a.jpeg
issue3916.jpeg
issue3976.jpeg
issue4084.jpeg
issue4259.jpeg
issue4291.jpeg
issue4337.jpeg
issue4500.jpeg
issue4705.jpeg
issue4975.jpeg
issue5112.jpeg
issue6767.jpeg
issue9888.jpeg
issue10133.jpeg
issue10357.jpeg
issue10447.jpeg
issue11648.jpeg
issue11650.jpeg

There were other images attached in the issue tracker that aren't
actually valid JPEGs. They failed both before and after this CL:
broken-issue2362b.jpeg
broken-issue6450.jpeg
broken-issue8693.jpeg
broken-issue10154.jpeg
broken-issue10387.jpeg
broken-issue10388.jpeg
broken-issue10389.jpeg
broken-issue10413.jpeg

In summary, this CL fixes #11650 and, after some automated and manual
testing, I don't think introduces new regressions.

Change-Id: I30b67036e9b087f3051d57dac7ea05fb4fa36f66
Reviewed-on: https://go-review.googlesource.com/12163
Reviewed-by: Rob Pike <r@golang.org>
---
 src/image/jpeg/huffman.go |  4 +++-
 src/image/jpeg/reader.go  | 17 ++++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/image/jpeg/huffman.go b/src/image/jpeg/huffman.go
index d4ff4cfa0c..4f8fe8eff3 100644
--- a/src/image/jpeg/huffman.go
+++ b/src/image/jpeg/huffman.go
@@ -187,7 +187,9 @@ func (d *decoder) decodeHuffman(h *huffman) (uint8, error) {
 			// There are no more bytes of data in this segment, but we may still
 			// be able to read the next symbol out of the previously read bits.
 			// First, undo the readByte that the ensureNBits call made.
-			d.unreadByteStuffedByte()
+			if d.bytes.nUnreadable != 0 {
+				d.unreadByteStuffedByte()
+			}
 			goto slowPath
 		}
 	}
diff --git a/src/image/jpeg/reader.go b/src/image/jpeg/reader.go
index 0fe57006bb..adf97abbd1 100644
--- a/src/image/jpeg/reader.go
+++ b/src/image/jpeg/reader.go
@@ -169,9 +169,6 @@ func (d *decoder) fill() error {
 // sometimes overshoot and read one or two too many bytes. Two-byte overshoot
 // can happen when expecting to read a 0xff 0x00 byte-stuffed byte.
 func (d *decoder) unreadByteStuffedByte() {
-	if d.bytes.nUnreadable == 0 {
-		return
-	}
 	d.bytes.i -= d.bytes.nUnreadable
 	d.bytes.nUnreadable = 0
 	if d.bits.n >= 8 {
@@ -243,7 +240,12 @@ func (d *decoder) readByteStuffedByte() (x byte, err error) {
 // stuffing.
 func (d *decoder) readFull(p []byte) error {
 	// Unread the overshot bytes, if any.
-	d.unreadByteStuffedByte()
+	if d.bytes.nUnreadable != 0 {
+		if d.bits.n >= 8 {
+			d.unreadByteStuffedByte()
+		}
+		d.bytes.nUnreadable = 0
+	}
 
 	for {
 		n := copy(p, d.bytes.buf[d.bytes.i:d.bytes.j])
@@ -265,7 +267,12 @@ func (d *decoder) readFull(p []byte) error {
 // ignore ignores the next n bytes.
 func (d *decoder) ignore(n int) error {
 	// Unread the overshot bytes, if any.
-	d.unreadByteStuffedByte()
+	if d.bytes.nUnreadable != 0 {
+		if d.bits.n >= 8 {
+			d.unreadByteStuffedByte()
+		}
+		d.bytes.nUnreadable = 0
+	}
 
 	for {
 		m := d.bytes.j - d.bytes.i
-- 
2.50.0