image/png: fix some 32-bit int overflows

Fixes #38435

Change-Id: Ib9ae3cf7f338b2860a5688e448a125f257fe624e
Reviewed-on: https://go-review.googlesource.com/c/go/+/230219
Reviewed-by: Andrew Ekstedt <andrew.ekstedt@gmail.com>
Reviewed-by: Rob Pike <r@golang.org>

2 files changed, 127 insertions, 17 deletions

diff --git a/src/image/png/reader.go b/src/image/png/reader.go
index 6771973bda..5521b39bb0 100644
--- a/src/image/png/reader.go
+++ b/src/image/png/reader.go
@@ -163,8 +163,9 @@ func (d *decoder) parseIHDR(length uint32) error {
 	if w <= 0 || h <= 0 {
 		return FormatError("non-positive dimension")
 	}
-	nPixels := int64(w) * int64(h)
-	if nPixels != int64(int(nPixels)) {
+	nPixels64 := int64(w) * int64(h)
+	nPixels := int(nPixels64)
+	if nPixels64 != int64(nPixels) {
 		return UnsupportedError("dimension overflow")
 	}
 	// There can be up to 8 bytes per pixel, for 16 bits per channel RGBA.
@@ -498,7 +499,10 @@ func (d *decoder) readImagePass(r io.Reader, pass int, allocateOnly bool) (image
 	bytesPerPixel := (bitsPerPixel + 7) / 8
 
 	// The +1 is for the per-row filter type, which is at cr[0].
-	rowSize := 1 + (bitsPerPixel*width+7)/8
+	rowSize := 1 + (int64(bitsPerPixel)*int64(width)+7)/8
+	if rowSize != int64(int(rowSize)) {
+		return nil, UnsupportedError("dimension overflow")
+	}
 	// cr and pr are the bytes for the current and previous row.
 	cr := make([]uint8, rowSize)
 	pr := make([]uint8, rowSize)
diff --git a/src/image/png/reader_test.go b/src/image/png/reader_test.go
index 3325d2e8a5..22c704e5cb 100644
--- a/src/image/png/reader_test.go
+++ b/src/image/png/reader_test.go
@@ -661,20 +661,126 @@ func TestGray8Transparent(t *testing.T) {
 }
 
 func TestDimensionOverflow(t *testing.T) {
-	// These bytes come from https://golang.org/issues/22304
-	//
-	// It encodes a 2147483646 × 2147483646 (i.e. 0x7ffffffe × 0x7ffffffe)
-	// NRGBA image. The (width × height) per se doesn't overflow an int64, but
-	// (width × height × bytesPerPixel) will.
-	_, err := Decode(bytes.NewReader([]byte{
-		0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
-		0x7f, 0xff, 0xff, 0xfe, 0x7f, 0xff, 0xff, 0xfe, 0x08, 0x06, 0x00, 0x00, 0x00, 0x30, 0x57, 0xb3,
-		0xfd, 0x00, 0x00, 0x00, 0x15, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x62, 0x62, 0x20, 0x12, 0x8c,
-		0x2a, 0xa4, 0xb3, 0x42, 0x40, 0x00, 0x00, 0x00, 0xff, 0xff, 0x13, 0x38, 0x00, 0x15, 0x2d, 0xef,
-		0x5f, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
-	}))
-	if _, ok := err.(UnsupportedError); !ok {
-		t.Fatalf("Decode: got %v (of type %T), want non-nil error (of type png.UnsupportedError)", err, err)
+	maxInt32AsInt := int((1 << 31) - 1)
+	have32BitInts := 0 > (1 + maxInt32AsInt)
+
+	testCases := []struct {
+		src               []byte
+		unsupportedConfig bool
+		width             int
+		height            int
+	}{
+		// These bytes come from https://golang.org/issues/22304
+		//
+		// It encodes a 2147483646 × 2147483646 (i.e. 0x7ffffffe × 0x7ffffffe)
+		// NRGBA image. The (width × height) per se doesn't overflow an int64, but
+		// (width × height × bytesPerPixel) will.
+		{
+			src: []byte{
+				0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
+				0x7f, 0xff, 0xff, 0xfe, 0x7f, 0xff, 0xff, 0xfe, 0x08, 0x06, 0x00, 0x00, 0x00, 0x30, 0x57, 0xb3,
+				0xfd, 0x00, 0x00, 0x00, 0x15, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x62, 0x62, 0x20, 0x12, 0x8c,
+				0x2a, 0xa4, 0xb3, 0x42, 0x40, 0x00, 0x00, 0x00, 0xff, 0xff, 0x13, 0x38, 0x00, 0x15, 0x2d, 0xef,
+				0x5f, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
+			},
+			// It's debatable whether DecodeConfig (which does not allocate a
+			// pixel buffer, unlike Decode) should fail in this case. The Go
+			// standard library has made its choice, and the standard library
+			// has compatibility constraints.
+			unsupportedConfig: true,
+			width:             0x7ffffffe,
+			height:            0x7ffffffe,
+		},
+
+		// The next three cases come from https://golang.org/issues/38435
+
+		{
+			src: []byte{
+				0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
+				0x00, 0x00, 0xb5, 0x04, 0x00, 0x00, 0xb5, 0x04, 0x08, 0x06, 0x00, 0x00, 0x00, 0xf5, 0x60, 0x2c,
+				0xb8, 0x00, 0x00, 0x00, 0x15, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x62, 0x62, 0x20, 0x12, 0x8c,
+				0x2a, 0xa4, 0xb3, 0x42, 0x40, 0x00, 0x00, 0x00, 0xff, 0xff, 0x13, 0x38, 0x00, 0x15, 0x2d, 0xef,
+				0x5f, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
+			},
+			// Here, width * height = 0x7ffea810, just under MaxInt32, but at 4
+			// bytes per pixel, the number of pixels overflows an int32.
+			unsupportedConfig: have32BitInts,
+			width:             0x0000b504,
+			height:            0x0000b504,
+		},
+
+		{
+			src: []byte{
+				0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
+				0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x06, 0x00, 0x00, 0x00, 0x30, 0x6e, 0xc5,
+				0x21, 0x00, 0x00, 0x00, 0x15, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x62, 0x62, 0x20, 0x12, 0x8c,
+				0x2a, 0xa4, 0xb3, 0x42, 0x40, 0x00, 0x00, 0x00, 0xff, 0xff, 0x13, 0x38, 0x00, 0x15, 0x2d, 0xef,
+				0x5f, 0x0f, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e, 0x44, 0xae, 0x42, 0x60, 0x82,
+			},
+			unsupportedConfig: false,
+			width:             0x04000000,
+			height:            0x00000001,
+		},
+
+		{
+			src: []byte{
+				0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
+				0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x06, 0x00, 0x00, 0x00, 0xaa, 0xd4, 0x7c,
+				0xda, 0x00, 0x00, 0x00, 0x15, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x62, 0x66, 0x20, 0x12, 0x30,
+				0x8d, 0x2a, 0xa4, 0xaf, 0x42, 0x40, 0x00, 0x00, 0x00, 0xff, 0xff, 0x14, 0xd2, 0x00, 0x16, 0x00,
+				0x00, 0x00,
+			},
+			unsupportedConfig: false,
+			width:             0x08000000,
+			height:            0x00000001,
+		},
+	}
+
+	for i, tc := range testCases {
+		cfg, err := DecodeConfig(bytes.NewReader(tc.src))
+		if tc.unsupportedConfig {
+			if err == nil {
+				t.Errorf("i=%d: DecodeConfig: got nil error, want non-nil", i)
+			} else if _, ok := err.(UnsupportedError); !ok {
+				t.Fatalf("Decode: got %v (of type %T), want non-nil error (of type png.UnsupportedError)", err, err)
+			}
+			continue
+		} else if err != nil {
+			t.Errorf("i=%d: DecodeConfig: %v", i, err)
+			continue
+		} else if cfg.Width != tc.width {
+			t.Errorf("i=%d: width: got %d, want %d", i, cfg.Width, tc.width)
+			continue
+		} else if cfg.Height != tc.height {
+			t.Errorf("i=%d: height: got %d, want %d", i, cfg.Height, tc.height)
+			continue
+		}
+
+		if nPixels := int64(cfg.Width) * int64(cfg.Height); nPixels > 0x7f000000 {
+			// In theory, calling Decode would succeed, given several gigabytes
+			// of memory. In practice, trying to make a []uint8 big enough to
+			// hold all of the pixels can often result in OOM (out of memory).
+			// OOM is unrecoverable; we can't write a test that passes when OOM
+			// happens. Instead we skip the Decode call (and its tests).
+			continue
+		} else if testing.Short() {
+			// Even for smaller image dimensions, calling Decode might allocate
+			// 1 GiB or more of memory. This is usually feasible, and we want
+			// to check that calling Decode doesn't panic if there's enough
+			// memory, but we provide a runtime switch (testing.Short) to skip
+			// these if it would OOM. See also http://golang.org/issue/5050
+			// "decoding... images can cause huge memory allocations".
+			continue
+		}
+
+		// Even if we don't panic, these aren't valid PNG images.
+		if _, err := Decode(bytes.NewReader(tc.src)); err == nil {
+			t.Errorf("i=%d: Decode: got nil error, want non-nil", i)
+		}
+	}
+
+	if testing.Short() {
+		t.Skip("skipping tests which allocate large pixel buffers")
 	}
 }