]> Cypherpunks repositories - gostls13.git/commitdiff
image/png: fix some 32-bit int overflows
authorNigel Tao <nigeltao@golang.org>
Mon, 27 Apr 2020 02:23:26 +0000 (12:23 +1000)
committerNigel Tao <nigeltao@golang.org>
Mon, 27 Apr 2020 11:55:27 +0000 (11:55 +0000)
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>
src/image/png/reader.go
src/image/png/reader_test.go

index 6771973bda9f6cd0b3a1a45f20db111b3b7bbe3b..5521b39bb0cd2e43e09857c23e1c089dc7316347 100644 (file)
@@ -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)
index 3325d2e8a59bb906d563ef34ae4b43413c04a9ca..22c704e5cba693066753b0c6f6f289336289c69b 100644 (file)
@@ -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")
        }
 }