From 8aad2278a0a93c18da7da36ee6638d34f7f044ba Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 22 Jul 2026 01:35:19 -0400 Subject: [PATCH] Address EXIF review: single alloc, no per-pixel boxing, hardened parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gadfly on #103: - (4 models) applyOrientation allocated a WxH RGBA then threw it away for a quarter-turn to allocate HxW. Compute the output dims once, allocate one buffer. - (perf) The At/Set loop boxed a color.Color per pixel — millions of heap allocs on a full-res rotation. Convert to *image.RGBA once (draw.Draw) then copy 4 bytes per pixel by offset. No boxing. - (2 findings) exifOrientation didn't skip 0xFF fill bytes before a marker, so a spec-valid padded APP1 would be misread. Skip them. - (2 findings) orientationFromApp1 read the tag's inline value without checking its type/count — a mistyped LONG/offset would be read as a bogus SHORT. Require type=SHORT, count=1; also validate the TIFF 0x2A magic agrees with the byte order. - Tests now cover all 8 orientations (added the mirror/transpose cases 2/4/5/7, the most error-prone switch arms) as t.Run subtests. All imagenorm tests green; gofmt clean; still CGO_ENABLED=0. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/imagenorm/imagenorm.go | 57 ++++++++++++++++++++++------ internal/imagenorm/imagenorm_test.go | 54 ++++++++++++++------------ 2 files changed, 75 insertions(+), 36 deletions(-) diff --git a/internal/imagenorm/imagenorm.go b/internal/imagenorm/imagenorm.go index 701b377..c19f667 100644 --- a/internal/imagenorm/imagenorm.go +++ b/internal/imagenorm/imagenorm.go @@ -202,22 +202,34 @@ func decodeSafely(raw []byte) (img image.Image, format string, err error) { // applyOrientation returns img with the EXIF orientation (1..8) baked in, so the // pixels are upright and no display-time rotation is needed. Orientation 1 (and // anything out of range) is a no-op. Values 5..8 are 90° rotations, which swap -// the output's width and height. Reads through image.Image.At and writes an RGBA, -// so it works whatever concrete type decode/downscale produced. +// the output's width and height. Copies raw RGBA pixels by byte offset (after a +// one-time conversion if the source isn't already RGBA), so a full-resolution +// rotation doesn't box a color.Color per pixel. func applyOrientation(img image.Image, o int) image.Image { if o <= 1 || o > 8 { return img } - b := img.Bounds() - w, h := b.Dx(), b.Dy() - swap := o >= 5 // a quarter-turn: output dimensions transpose - dst := image.NewRGBA(image.Rect(0, 0, w, h)) - if swap { - dst = image.NewRGBA(image.Rect(0, 0, h, w)) + // Work on a concrete RGBA so the transform is a 4-byte copy per pixel rather + // than millions of boxed color.Color values through At/Set. downscale usually + // hands us an *image.RGBA already; convert once if not (e.g. a small JPEG that + // skipped downscale decodes to YCbCr). + src, ok := img.(*image.RGBA) + if !ok { + b := img.Bounds() + conv := image.NewRGBA(image.Rect(0, 0, b.Dx(), b.Dy())) + draw.Draw(conv, conv.Bounds(), img, b.Min, draw.Src) + src = conv } + sb := src.Bounds() + w, h := sb.Dx(), sb.Dy() + // A quarter-turn (5..8) transposes the output; size the one buffer accordingly. + dw, dh := w, h + if o >= 5 { + dw, dh = h, w + } + dst := image.NewRGBA(image.Rect(0, 0, dw, dh)) for y := range h { for x := range w { - c := img.At(b.Min.X+x, b.Min.Y+y) var dx, dy int switch o { case 2: // mirror horizontal @@ -235,7 +247,9 @@ func applyOrientation(img image.Image, o int) image.Image { case 8: // rotate 90° counter-clockwise dx, dy = y, w-1-x } - dst.Set(dx, dy, c) + si := src.PixOffset(sb.Min.X+x, sb.Min.Y+y) + di := dst.PixOffset(dx, dy) + copy(dst.Pix[di:di+4], src.Pix[si:si+4]) } } return dst @@ -252,14 +266,25 @@ func exifOrientation(raw []byte) int { if len(raw) < 4 || raw[0] != 0xFF || raw[1] != 0xD8 { return 1 } - for i := 2; i+4 <= len(raw); { + for i := 2; i+1 < len(raw); { if raw[i] != 0xFF { return 1 // not aligned on a marker; give up rather than misread } + // A marker may be preceded by any number of 0xFF fill bytes (JPEG spec); + // skip them so a padded APP1 isn't misread as a marker of value 0xFF. + for i+1 < len(raw) && raw[i+1] == 0xFF { + i++ + } + if i+1 >= len(raw) { + return 1 + } marker := raw[i+1] if marker == 0xD9 || marker == 0xDA { return 1 // EOI / start-of-scan: no more headers to read } + if i+4 > len(raw) { + return 1 + } segLen := int(raw[i+2])<<8 | int(raw[i+3]) if segLen < 2 || i+2+segLen > len(raw) { return 1 @@ -292,6 +317,9 @@ func orientationFromApp1(seg []byte) (int, bool) { default: return 0, false } + if bo.Uint16(tiff[2:4]) != 0x2A { // TIFF magic (42); byte order must agree + return 0, false + } ifd := int(bo.Uint32(tiff[4:8])) // offset to IFD0 from the TIFF start if ifd < 8 || ifd+2 > len(tiff) { return 0, false @@ -305,7 +333,12 @@ func orientationFromApp1(seg []byte) (int, bool) { if bo.Uint16(tiff[off:off+2]) != 0x0112 { // Orientation tag continue } - // SHORT value lives in the first 2 bytes of the entry's value field. + // Orientation is defined as a single SHORT, whose value sits inline in the + // first 2 bytes of the value field. Reject anything else rather than read a + // mistyped entry (a LONG/offset there would be a different number entirely). + if bo.Uint16(tiff[off+2:off+4]) != 3 || bo.Uint32(tiff[off+4:off+8]) != 1 { + return 0, false + } v := int(bo.Uint16(tiff[off+8 : off+10])) if v >= 1 && v <= 8 { return v, true diff --git a/internal/imagenorm/imagenorm_test.go b/internal/imagenorm/imagenorm_test.go index acf440e..3b804bb 100644 --- a/internal/imagenorm/imagenorm_test.go +++ b/internal/imagenorm/imagenorm_test.go @@ -269,36 +269,42 @@ func TestNormalizeAppliesExifOrientation(t *testing.T) { // orientation, wantW/H is the corrected canvas and (mx,my) is where that // marker must land — derived from the same transform Normalize applies. cases := []struct { + name string orient int wantW, wantH int mx, my int }{ - {0, 40, 24, 10, 6}, // no EXIF → unchanged, marker top-left - {1, 40, 24, 10, 6}, // normal → unchanged - {3, 40, 24, 29, 17}, // 180 → bottom-right - {6, 24, 40, 17, 10}, // 90° CW → top-right (dims swap) - {8, 24, 40, 6, 29}, // 90° CCW → bottom-left (dims swap) + {"none", 0, 40, 24, 10, 6}, // no EXIF → unchanged, marker top-left + {"normal", 1, 40, 24, 10, 6}, // normal → unchanged + {"mirror-h", 2, 40, 24, 29, 6}, // flip horizontal → top-right + {"rotate-180", 3, 40, 24, 29, 17}, // → bottom-right + {"mirror-v", 4, 40, 24, 10, 17}, // flip vertical → bottom-left + {"transpose", 5, 24, 40, 6, 10}, // main diagonal (dims swap) + {"rotate-90-cw", 6, 24, 40, 17, 10}, // → top-right (dims swap) + {"transverse", 7, 24, 40, 17, 29}, // anti-diagonal (dims swap) + {"rotate-90-ccw", 8, 24, 40, 6, 29}, // → bottom-left (dims swap) } for _, tc := range cases { - out, format, err := Normalize(bytes.NewReader(orientedJPEG(t, tc.orient)), Options{}) - if err != nil { - t.Fatalf("orient %d: Normalize err = %v", tc.orient, err) - } - if format != "jpeg" { - t.Errorf("orient %d: format = %q, want jpeg", tc.orient, format) - } - m, _, err := image.Decode(bytes.NewReader(out)) - if err != nil { - t.Fatalf("orient %d: decode output: %v", tc.orient, err) - } - if m.Bounds().Dx() != tc.wantW || m.Bounds().Dy() != tc.wantH { - t.Errorf("orient %d: output %dx%d, want %dx%d", - tc.orient, m.Bounds().Dx(), m.Bounds().Dy(), tc.wantW, tc.wantH) - } - if !bright(m.At(m.Bounds().Min.X+tc.mx, m.Bounds().Min.Y+tc.my)) { - t.Errorf("orient %d: white marker not at (%d,%d) — orientation not applied", - tc.orient, tc.mx, tc.my) - } + t.Run(tc.name, func(t *testing.T) { + out, format, err := Normalize(bytes.NewReader(orientedJPEG(t, tc.orient)), Options{}) + if err != nil { + t.Fatalf("Normalize err = %v", err) + } + if format != "jpeg" { + t.Errorf("format = %q, want jpeg", format) + } + m, _, err := image.Decode(bytes.NewReader(out)) + if err != nil { + t.Fatalf("decode output: %v", err) + } + if m.Bounds().Dx() != tc.wantW || m.Bounds().Dy() != tc.wantH { + t.Errorf("output %dx%d, want %dx%d", + m.Bounds().Dx(), m.Bounds().Dy(), tc.wantW, tc.wantH) + } + if !bright(m.At(m.Bounds().Min.X+tc.mx, m.Bounds().Min.Y+tc.my)) { + t.Errorf("white marker not at (%d,%d) — orientation not applied", tc.mx, tc.my) + } + }) } }