From 6f9b1fc47d7bf764d4a02806bab4c4978b9fbc47 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 4 Nov 2026 16:37:46 +0100 Subject: [PATCH 1/4] feat: a ceiling on what a PNG may ask DrawPNG to allocate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #43. A PNG's HEADER decides the allocation. IHDR's width and height are four-byte unsigned integers (W3C PNG 3rd ed. §11.2.1) and nothing in the file has to back them up: seventy-five bytes claiming 20000x20000 cost 1.5 GiB before png.Decode reads a single sample. The identical shape in go-pdfkit/convert held 381.6 MiB for a 10000x10000 claim. DrawPNG now reads IHDR with png.DecodeConfig and refuses past Options.MaxPixels BEFORE decoding -- a ceiling applied to what a decoder returns is not a ceiling, it is a post-mortem. The spec asks for the same order: "PNG decoders should detect errors as early as possible" (§4.10). The product is taken in int64 because both factors are the file's and range over 0..2^31-1. Options.MaxPixels defaults to DefaultMaxPixels = 40_000_000, the number go-pdfkit/render and go-gfx/gfx's svg package already use: a hundred and sixty million bytes, 152 MiB, at the four bytes a pixel image/png returns. ⛔ WHAT IS DELIBERATELY NOT BOUNDED, because "it has a limit" otherwise reads as "everything has one": - DrawJPEG decodes nothing. The bytes become a DCTDecode stream and the geometry comes from SOF, so a hundred-megapixel scan costs its own length. A ceiling there would refuse real documents to prevent no allocation. - DrawImage is handed pixels the caller already holds; refusing them saves nothing and it copies them once more, a constant factor. The w*h*3 in samples() would overflow int on a 32-bit build only past 715 megapixels -- 2.8 GB of pixel buffer the caller could not have addressed to get here. ⛔ The post-decode check go-pdfkit/convert#4 also makes is NOT repeated here: png.DecodeConfig and png.Decode take their dimensions from the same IHDR, so a second comparison could not differ, and an unreachable branch is a comfort rather than a check. That is an assumption about image/png, not a theorem, so TestTheDecodedSizeIsTheSizeTheHeaderDeclared holds it up: if a Go release ever makes the two disagree, it goes red rather than the gap being found through a 1.5 GiB allocation. ⛔⛔ THE BOMB TEST CARRIES ITS OWN CONTROL. A child process caps its own address space at 1 GiB (RLIMIT_AS, set in the child because os/exec has no rlimit) and must refuse the bomb and exit cleanly. A clean exit proves nothing on its own, so a second child decodes the same bytes with no ceiling under the same limit and MUST die: if it ever survives, the limit is not being applied and the first child's exit meant nothing. macOS rejects RLIMIT_AS outright, so there both children skip -- loudly, naming the platform, with the heap measurement left to carry the proof. Co-Authored-By: Claude Opus 5 --- README.md | 37 +++++++ ceiling_test.go | 240 +++++++++++++++++++++++++++++++++++++++++++ document.go | 30 ++++++ image.go | 27 ++++- rlimit_other_test.go | 15 +++ rlimit_unix_test.go | 28 +++++ 6 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 ceiling_test.go create mode 100644 rlimit_other_test.go create mode 100644 rlimit_unix_test.go diff --git a/README.md b/README.md index d4a39fd..bc3a96d 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,8 @@ Go-idiomatic rather than a gem port. **Pixel-identical bitmaps are shared**: each image is content-addressed by its *uncompressed* samples, so a repeat is embedded — and compressed — once per document and every placement, on any page, references the one XObject. + `DrawPNG` reads IHDR before it decodes and refuses a picture declaring more + than `Options.MaxPixels` — see below. - **Form XObjects** (ISO 32000-1 §8.10) — `Document.NewForm(bbox)` returns a `Form`, drawn with every `Page` method (paths, colours, alpha, shadings, images, text, other forms) in its own form space. `Page.DrawForm(f, rect)` @@ -70,6 +72,41 @@ Go-idiomatic rather than a gem port. - **Deterministic** — with the zero `Options`, output has no timestamps and a content-derived `/ID`, so identical inputs produce byte-identical PDFs. +## What a PNG may ask for + +A PNG's **header** decides the allocation. IHDR's width and height are +four-byte unsigned integers (W3C PNG 3rd ed. §11.2.1), and nothing in the file +has to back them up: **seventy-five bytes** claiming 20000×20000 cost +**1.5 GiB** before `png.Decode` looks at a single sample. Measured on the +identical shape in `go-pdfkit/convert`, a 10000×10000 claim held 381.6 MiB. + +`DrawPNG` now reads IHDR with `png.DecodeConfig` and refuses past +`Options.MaxPixels` — ⛔ *before* decoding, because a ceiling applied to what a +decoder returns is not a ceiling, it is a post-mortem. The spec asks for the +same reading order: "PNG decoders should detect errors as early as possible" +(§4.10). + +`Options.MaxPixels` defaults to `DefaultMaxPixels`, **forty million pixels** — +a hundred and sixty million bytes, 152 MiB, at the four bytes a pixel +`image/png` hands back. That is the ceiling `go-pdfkit/render` and +`go-gfx/gfx`'s `svg` package already use, and a little over A4 at 600 dpi. A +program embedding its own logo never meets it; a program embedding pictures it +did not produce should set it to what its own documents need. + +Two neighbours are deliberately **not** bounded, and the reasons are worth +stating because "it has a limit" is otherwise read as "everything has a limit": + +- **`DrawJPEG`** decodes nothing. The bytes are embedded as a DCTDecode stream + and the geometry is read from the SOF marker, so a hundred-megapixel scan + costs pdfkit its own length and no more. Its dimensions are not a lie a + consumer will discover later, either: the consumer decodes the same JPEG. +- **`DrawImage`** is handed an `image.Image` the caller has already allocated, + so the pixels exist before pdfkit sees them and refusing them saves nothing. + It copies them once more, a constant factor, not an amplification. (On a + 32-bit build `w*h*3` would overflow `int` — but only for an image past 715 + megapixels, which is 2.8 GB of pixel buffer the caller could not have + addressed to get here.) + ## Install ```sh diff --git a/ceiling_test.go b/ceiling_test.go new file mode 100644 index 0000000..b6d9406 --- /dev/null +++ b/ceiling_test.go @@ -0,0 +1,240 @@ +// Copyright (c) 2026 the go-pdfkit/pdfkit authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file at the root of this repository. + +package pdfkit + +import ( + "bytes" + "fmt" + "hash/crc32" + "image" + "image/png" + "os" + "os/exec" + "runtime" + "strings" + "testing" +) + +// claimingPNG is a real one-pixel PNG whose IHDR has been rewritten to claim w +// by h, with the chunk's CRC recomputed so every decoder accepts the header. +// The pixel data is still one pixel. +// +// ⛔ This is what the attack looks like: not large, not malformed, and nothing +// about it suspicious until something multiplies its two numbers together. +func claimingPNG(t testing.TB, w, h uint32) []byte { + t.Helper() + var buf bytes.Buffer + if err := png.Encode(&buf, image.NewNRGBA(image.Rect(0, 0, 1, 1))); err != nil { + t.Fatal(err) + } + b := buf.Bytes() + // 8 signature, then IHDR: 4 length, 4 type, 13 data (width then height), + // 4 CRC over type+data. W3C PNG 3rd ed. §11.2.1. + put32(b[16:], w) + put32(b[20:], h) + put32(b[29:], crc32.ChecksumIEEE(b[12:29])) + return b +} + +func put32(b []byte, v uint32) { + b[0], b[1], b[2], b[3] = byte(v>>24), byte(v>>16), byte(v>>8), byte(v) +} + +// heldDuring is how much the heap grew across f, with a collection either +// side. Coarse, which is all this needs: the difference is between zero and +// thousands of megabytes. +func heldDuring(f func()) float64 { + runtime.GC() + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + f() + runtime.ReadMemStats(&after) + if after.HeapAlloc < before.HeapAlloc { + return 0 + } + return float64(after.HeapAlloc-before.HeapAlloc) / (1 << 20) +} + +func TestAPNGIsRefusedFromItsHeaderBeforeItsPixelsAreAllocated(t *testing.T) { + // ⛔ The file is not large, not malformed, and not slow. Measured on the + // identical shape in go-pdfkit/convert: a 10000x10000 claim held 381.6 + // MiB; the four hundred megapixels claimed here are 1.5 GiB. + bomb := claimingPNG(t, 20000, 20000) + if len(bomb) > 200 { + t.Fatalf("the fixture is %d bytes, which is not the point being made", len(bomb)) + } + d := New(Options{}) + p := d.AddPage(A4) + var err error + held := heldDuring(func() { err = p.DrawPNG(bomb, Rect{Width: 100, Height: 100}) }) + if err == nil { + t.Fatal("a PNG claiming four hundred megapixels was decoded") + } + if !strings.Contains(err.Error(), "pixels") { + t.Errorf("it was refused as %q, which is not a refusal about its size", err) + } + if held > 32 { + t.Errorf("refusing it held %.1f MiB: the ceiling is being applied after the "+ + "allocation it is meant to bound", held) + } +} + +func TestAnOrdinaryPictureIsStillEmbedded(t *testing.T) { + // ⛔ The side a ceiling gets wrong. Tested only from above, a limit is + // satisfied by refusing everything, and the test above would not notice. + d := New(Options{}) + p := d.AddPage(A4) + if err := p.DrawPNG(pngBytes(makeTestImage()), Rect{Width: 5, Height: 5}); err != nil { + t.Fatalf("an ordinary picture was refused: %v", err) + } + if len(d.images) != 1 { + t.Errorf("the document holds %d images: it was accepted and then not embedded", len(d.images)) + } +} + +func TestTheCeilingCanBeRaisedAndLowered(t *testing.T) { + img := claimingPNG(t, 1000, 1000) // a claim, so no megabytes are spent proving this + draw := func(max int) error { + d := New(Options{MaxPixels: max}) + return d.AddPage(A4).DrawPNG(img, Rect{Width: 10, Height: 10}) + } + if err := draw(999_999); err == nil || !strings.Contains(err.Error(), "pixels") { + t.Errorf("a million pixels passed a ceiling of 999999: %v", err) + } + // Exactly at the ceiling passes the ceiling; it then fails to decode, + // because the pixel data is still one pixel. Two different refusals, and + // the point is which one this is. + if err := draw(1_000_000); err != nil && strings.Contains(err.Error(), "pixels") { + t.Errorf("exactly at the ceiling was refused for its size: %v", err) + } + // ⛔ Zero means the default, not "no pixels allowed". Options{} is the + // ordinary case — a caller who said nothing about size — and reading it as + // a ceiling of zero would refuse every picture over a limit nobody set. + if err := draw(0); err != nil && strings.Contains(err.Error(), "pixels") { + t.Errorf("a picture was refused for its size when no ceiling was given: %v", err) + } +} + +func TestTheDecodedSizeIsTheSizeTheHeaderDeclared(t *testing.T) { + // ⛔ This test exists to hold up an omission. go-pdfkit/convert#4 checks + // the size again after decoding, because in general a header is a claim. + // DrawPNG does not, on the grounds that png.DecodeConfig and png.Decode + // both take their dimensions from the same IHDR — so the second check + // could not differ, and an unreachable branch is a comfort, not a check. + // + // That is an assumption about image/png, not a theorem. If a Go release + // ever makes the two disagree, this goes red and the omission is revisited + // rather than discovered through a 1.5 GiB allocation. + data := pngBytes(makeTestImage()) + cfg, err := png.DecodeConfig(bytes.NewReader(data)) + if err != nil { + t.Fatal(err) + } + img, err := png.Decode(bytes.NewReader(data)) + if err != nil { + t.Fatal(err) + } + if b := img.Bounds(); b.Dx() != cfg.Width || b.Dy() != cfg.Height { + t.Errorf("the header declared %dx%d and the decoder returned %dx%d: DrawPNG's "+ + "header check no longer bounds what DrawPNG decodes", + cfg.Width, cfg.Height, b.Dx(), b.Dy()) + } +} + +func TestTheDefaultCeilingIsTheNumberTheDocumentationQuotes(t *testing.T) { + // ⛔ A change detector, deliberately. The number is quoted in Options's doc + // comment, in DefaultMaxPixels's, and in the README as a figure in + // megabytes — moving it silently makes three statements false at once, and + // the only one anybody notices is the one that stops a real document. + // + // Written out rather than derived from the constant: deriving it would + // make the judge move with the subject, which is how a ceiling test passes + // a mutation that raised the ceiling. + if DefaultMaxPixels != 40_000_000 { + t.Errorf("DefaultMaxPixels is %d. If that was meant, update Options.MaxPixels's "+ + "doc comment, DefaultMaxPixels's, and the README.", DefaultMaxPixels) + } + if got := fmt.Sprintf("%d MiB", 40_000_000*4/(1<<20)); got != "152 MiB" { + t.Errorf("forty million pixels at four bytes each is %s", got) + } +} + +// --- the bomb, under a hard ceiling on the whole process ------------------ + +// bombEnv selects the child process's half of the bomb test: "guarded" calls +// DrawPNG, "bare" calls png.Decode with nothing in the way. +const bombEnv = "PDFKIT_BOMB_CHILD" + +func TestABombIsRefusedByAProcessTooSmallToHoldIt(t *testing.T) { + // ⛔ The heap test above reads two numbers and believes them. This one + // cannot be argued with: the child is given an address space far smaller + // than the bomb's 1.5 GiB, so an unbounded decode does not merely allocate + // too much, it DIES. A refusal and a corpse are easy to tell apart. + // + // It also keeps the bomb off this machine's memory — the limit is the + // child's, not the host's, which matters on a box running two dozen + // sessions at once. + // + // ⛔⛔ And it carries its own control, because a child that exits cleanly + // proves nothing unless an unguarded one does not. The "bare" half decodes + // the same bytes with no ceiling under the same limit and MUST die; if it + // ever survives, the limit is not being applied and the "guarded" half's + // clean exit means nothing. + if mode := os.Getenv(bombEnv); mode != "" { + drawTheBomb(t, mode) + return + } + out, err := runBombChild(t, "guarded") + if err != nil { + t.Fatalf("the child did not survive the bomb under a 1 GiB address space, "+ + "which is what an unbounded decode looks like: %v\n%s", err, out) + } + if !bytes.Contains(out, []byte("refused:")) { + t.Errorf("the child finished without refusing the bomb:\n%s", out) + } + bare, err := runBombChild(t, "bare") + if err == nil { + t.Errorf("decoding the same bomb with NO ceiling also survived a 1 GiB address "+ + "space, so the limit is not being applied and the run above proved "+ + "nothing:\n%s", bare) + } +} + +// runBombChild re-runs this one test in a child process in the given mode. A +// child that could not take the limit skips the parent too, loudly: a bomb +// test that quietly runs without its limit is the thing the limit is for. +func runBombChild(t *testing.T, mode string) ([]byte, error) { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=TestABombIsRefusedByAProcessTooSmallToHoldIt", "-test.v") + cmd.Env = append(os.Environ(), bombEnv+"="+mode, "GOMEMLIMIT=256MiB") + out, err := cmd.CombinedOutput() + if bytes.Contains(out, []byte(skipMark)) { + t.Skipf("%s does not enforce an address-space limit, so the bomb is left to the "+ + "heap measurement in TestAPNGIsRefusedFromItsHeaderBeforeItsPixelsAreAllocated", + runtime.GOOS) + } + return out, err +} + +const skipMark = "no address-space limit here:" + +// drawTheBomb is the child half. The limit is taken HERE rather than handed +// down from the parent: os/exec has no rlimit of its own, and the child is our +// own code, so it can refuse itself the room before it is asked for any. +func drawTheBomb(t *testing.T, mode string) { + if err := capAddressSpace(1 << 30); err != nil { + t.Skipf("%s %v", skipMark, err) + } + bomb := claimingPNG(t, 20000, 20000) + if mode == "bare" { + img, err := png.Decode(bytes.NewReader(bomb)) + t.Fatalf("the control survived: png.Decode returned %v, %v", img != nil, err) + } + err := New(Options{}).AddPage(A4).DrawPNG(bomb, Rect{Width: 100, Height: 100}) + if err == nil { + t.Fatal("the bomb was decoded") + } + t.Logf("refused: %v", err) +} diff --git a/document.go b/document.go index ce81d7c..10234c9 100644 --- a/document.go +++ b/document.go @@ -72,11 +72,41 @@ type Options struct { // with Title, Lang and DisplayDocTitle set, for the claim to hold; the // rest, every drawing tagged or an artifact, is the caller's to keep. PDFUA bool + + // MaxPixels is the largest picture [Page.DrawPNG] will decode, counted in + // pixels. Zero means [DefaultMaxPixels]. + // + // ⛔ A PNG's HEADER decides the allocation, and the header is a claim the + // file makes about itself. Seventy-five bytes with a valid IHDR saying + // 20000x20000 cost 1.5 GiB before anything looks at the pixel data. A + // program embedding its own logo never meets this; a program embedding a + // picture it did not produce meets it on the first hostile upload, and + // should set this to what its own documents need rather than inherit the + // default. + MaxPixels int } // DefaultProducer is the /Producer value used when Options.Producer is empty. const DefaultProducer = "go-pdfkit/pdfkit" +// DefaultMaxPixels is the largest picture [Page.DrawPNG] decodes when +// [Options.MaxPixels] is zero. +// +// Said in bytes, because a count of pixels does not tell anybody what it +// permits: image/png hands back four bytes a pixel, so forty million pixels is +// a hundred and sixty MILLION BYTES — 152 MiB — before pdfkit has copied a +// single sample. That is the ceiling go-pdfkit/render and go-gfx/gfx's svg +// package already use, and it is a little over A4 at 600 dots to the inch. +const DefaultMaxPixels = 40_000_000 + +// maxPixels is the ceiling in force for this document. +func (d *Document) maxPixels() int { + if d.opts.MaxPixels <= 0 { + return DefaultMaxPixels + } + return d.opts.MaxPixels +} + // Document is a PDF document under construction. Build it with New, append // pages with AddPage, then serialise with Write. It is not safe for concurrent // use. diff --git a/image.go b/image.go index 108afde..613f68c 100644 --- a/image.go +++ b/image.go @@ -151,12 +151,37 @@ func unpremultiply(c []byte, a byte) { } // DrawPNG decodes PNG bytes and embeds the image into r. It returns an error if -// the data is not a valid PNG. +// the data is not a valid PNG, or if it declares more than +// [Options.MaxPixels] pixels. func (p *Page) DrawPNG(data []byte, r Rect) error { + // ⛔ THE HEADER FIRST. png.Decode allocates the whole pixel buffer from + // IHDR before it reads a byte of image data, so a ceiling applied to what + // it returns is not a ceiling, it is a post-mortem: a seventy-five byte + // file claiming 20000x20000 costs 1.5 GiB on the way to being rejected. + // The spec asks for exactly this reading order — "PNG decoders should + // detect errors as early as possible" (W3C PNG 3rd ed. §4.10). + // + // Multiplied in int64 because the two factors are not ours: IHDR's width + // and height are four-byte unsigned integers over 0..2^31-1 (§11.2.1), and + // two of those multiplied in int on a 32-bit build is how a ceiling is + // passed by overflowing past it. + cfg, err := png.DecodeConfig(bytes.NewReader(data)) + if err != nil { + return fmt.Errorf("pdfkit: decode png header: %w", err) + } + if px := int64(cfg.Width) * int64(cfg.Height); px > int64(p.doc.maxPixels()) { + return fmt.Errorf("pdfkit: a PNG of %dx%d is %d pixels, past the %d allowed", + cfg.Width, cfg.Height, px, p.doc.maxPixels()) + } img, err := png.Decode(bytes.NewReader(data)) if err != nil { return fmt.Errorf("pdfkit: decode png: %w", err) } + // go-pdfkit/convert#4 measures what came back as well, because in general + // a header is a claim. Not repeated here: both calls above read their + // dimensions from the same IHDR, so a second comparison could not differ, + // and an unreachable branch is a comfort rather than a check. + // TestTheDecodedSizeIsTheSizeTheHeaderDeclared holds that assumption. p.DrawImage(img, r) return nil } diff --git a/rlimit_other_test.go b/rlimit_other_test.go new file mode 100644 index 0000000..8c05375 --- /dev/null +++ b/rlimit_other_test.go @@ -0,0 +1,15 @@ +// Copyright (c) 2026 the go-pdfkit/pdfkit authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file at the root of this repository. + +//go:build !unix + +package pdfkit + +import "errors" + +// capAddressSpace has no counterpart outside the Unixes, so the bomb test +// skips rather than running without the limit that makes it safe. +func capAddressSpace(uint64) error { + return errors.New("no RLIMIT_AS on this platform") +} diff --git a/rlimit_unix_test.go b/rlimit_unix_test.go new file mode 100644 index 0000000..ce63c41 --- /dev/null +++ b/rlimit_unix_test.go @@ -0,0 +1,28 @@ +// Copyright (c) 2026 the go-pdfkit/pdfkit authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file at the root of this repository. + +//go:build unix + +package pdfkit + +import "syscall" + +// capAddressSpace lowers this process's address-space ceiling to n bytes, so a +// decode that asks for more dies here instead of on the machine running the +// tests. It never raises the ceiling: a limit already tighter than n is left +// alone, because something else set it for a reason. +func capAddressSpace(n uint64) error { + var cur syscall.Rlimit + if err := syscall.Getrlimit(syscall.RLIMIT_AS, &cur); err != nil { + return err + } + if cur.Cur != 0 && cur.Cur < n { + return nil + } + lim := syscall.Rlimit{Cur: n, Max: cur.Max} + if cur.Max != 0 && cur.Max < n { + lim.Cur = cur.Max + } + return syscall.Setrlimit(syscall.RLIMIT_AS, &lim) +} From 056480072db37523792b8943b3206dd0bc54f63f Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 4 Nov 2026 16:49:44 +0100 Subject: [PATCH 2/4] fix(test): cap the bomb child's address space RELATIVE to what it holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⛔ The first version of this test failed on CI in the words of the defect it was written to catch: "fatal error: runtime: out of memory". The stack named ceiling_test.go:29 -- claimingPNG, encoding a ONE-PIXEL fixture, long before DrawPNG was called. An absolute RLIMIT_AS of 1 GiB is not a limit on a Go program, it is a sentence. The runtime reserves a great deal of address space at startup -- arena hints and the page allocator's summaries -- so by the time the child could set a limit on itself it was already over this one, and every later mmap failed, including a 1x1 png.Encode. What a test can bound is what the program asks for FROM HERE. capAddressSpace now reads the process's current size from /proc/self/statm and allows it 512 MiB more: ample for the fixture and whatever the runtime wants next, and a third of what the bomb needs. The fixture is also built BEFORE the limit comes down, so the test cannot die making its own evidence. macOS rejects RLIMIT_AS outright (EINVAL), and the relative form needs /proc/self/statm, so the helper is now linux-only and everywhere else skips loudly -- naming the platform -- with the heap measurement left to carry the proof. The "bare" control child is unchanged and is what proves the limit bites: it decodes the same bytes with no ceiling under the same headroom and must die. Co-Authored-By: Claude Opus 5 --- ceiling_test.go | 22 +++++++++------ rlimit_linux_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++++ rlimit_other_test.go | 9 +++--- rlimit_unix_test.go | 28 ------------------ 4 files changed, 86 insertions(+), 40 deletions(-) create mode 100644 rlimit_linux_test.go delete mode 100644 rlimit_unix_test.go diff --git a/ceiling_test.go b/ceiling_test.go index b6d9406..0fd8f09 100644 --- a/ceiling_test.go +++ b/ceiling_test.go @@ -169,9 +169,10 @@ const bombEnv = "PDFKIT_BOMB_CHILD" func TestABombIsRefusedByAProcessTooSmallToHoldIt(t *testing.T) { // ⛔ The heap test above reads two numbers and believes them. This one - // cannot be argued with: the child is given an address space far smaller - // than the bomb's 1.5 GiB, so an unbounded decode does not merely allocate - // too much, it DIES. A refusal and a corpse are easy to tell apart. + // cannot be argued with: the child may reserve 512 MiB more address space + // than it already holds, a third of what the bomb needs, so an unbounded + // decode does not merely allocate too much, it DIES. A refusal and a + // corpse are easy to tell apart. // // It also keeps the bomb off this machine's memory — the limit is the // child's, not the host's, which matters on a box running two dozen @@ -188,7 +189,7 @@ func TestABombIsRefusedByAProcessTooSmallToHoldIt(t *testing.T) { } out, err := runBombChild(t, "guarded") if err != nil { - t.Fatalf("the child did not survive the bomb under a 1 GiB address space, "+ + t.Fatalf("the child did not survive the bomb with 512 MiB of headroom, "+ "which is what an unbounded decode looks like: %v\n%s", err, out) } if !bytes.Contains(out, []byte("refused:")) { @@ -196,8 +197,8 @@ func TestABombIsRefusedByAProcessTooSmallToHoldIt(t *testing.T) { } bare, err := runBombChild(t, "bare") if err == nil { - t.Errorf("decoding the same bomb with NO ceiling also survived a 1 GiB address "+ - "space, so the limit is not being applied and the run above proved "+ + t.Errorf("decoding the same bomb with NO ceiling also survived 512 MiB of "+ + "headroom, so the limit is not being applied and the run above proved "+ "nothing:\n%s", bare) } } @@ -224,10 +225,15 @@ const skipMark = "no address-space limit here:" // down from the parent: os/exec has no rlimit of its own, and the child is our // own code, so it can refuse itself the room before it is asked for any. func drawTheBomb(t *testing.T, mode string) { - if err := capAddressSpace(1 << 30); err != nil { + // The fixture is built BEFORE the limit comes down. It is one pixel, but + // encoding it asks the runtime for room, and a test that dies making its + // own fixture fails in the same words as the defect it is looking for. + bomb := claimingPNG(t, 20000, 20000) + // 512 MiB of headroom: ample for a 1x1 PNG and for whatever the runtime + // wants next, and a third of what this bomb needs. + if err := capAddressSpace(512 << 20); err != nil { t.Skipf("%s %v", skipMark, err) } - bomb := claimingPNG(t, 20000, 20000) if mode == "bare" { img, err := png.Decode(bytes.NewReader(bomb)) t.Fatalf("the control survived: png.Decode returned %v, %v", img != nil, err) diff --git a/rlimit_linux_test.go b/rlimit_linux_test.go new file mode 100644 index 0000000..c2442d3 --- /dev/null +++ b/rlimit_linux_test.go @@ -0,0 +1,67 @@ +// Copyright (c) 2026 the go-pdfkit/pdfkit authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file at the root of this repository. + +//go:build linux + +package pdfkit + +import ( + "fmt" + "os" + "strconv" + "syscall" +) + +// capAddressSpace lets this process reserve headroom more bytes of address +// space than it already has, and no more. +// +// ⛔ The ceiling is RELATIVE, and the first version of this was not. A Go +// program reserves a great deal of address space at startup — arena hints and +// the page allocator's summaries — so an absolute cap of 1 GiB did not bound +// the bomb, it killed the process while it was still encoding a ONE-PIXEL +// fixture. The failure looked exactly like the one the test is for: "fatal +// error: runtime: out of memory", from a test about running out of memory. +// +// What a test can bound is what the program asks for FROM HERE, so that is +// what this bounds. +func capAddressSpace(headroom uint64) error { + size, err := virtualSize() + if err != nil { + return err + } + var cur syscall.Rlimit + if err := syscall.Getrlimit(syscall.RLIMIT_AS, &cur); err != nil { + return err + } + want := size + headroom + if cur.Cur != ^uint64(0) && cur.Cur < want { + return nil // something tighter is already in force; leave it be + } + lim := syscall.Rlimit{Cur: want, Max: cur.Max} + if cur.Max != ^uint64(0) && cur.Max < want { + lim.Cur = cur.Max + } + return syscall.Setrlimit(syscall.RLIMIT_AS, &lim) +} + +// virtualSize is this process's current address-space size in bytes, the first +// field of /proc/self/statm counted in pages. +func virtualSize() (uint64, error) { + b, err := os.ReadFile("/proc/self/statm") + if err != nil { + return 0, err + } + field := string(b) + for i := range len(field) { + if field[i] == ' ' { + field = field[:i] + break + } + } + pages, err := strconv.ParseUint(field, 10, 64) + if err != nil { + return 0, fmt.Errorf("/proc/self/statm began %q: %w", field, err) + } + return pages * uint64(os.Getpagesize()), nil +} diff --git a/rlimit_other_test.go b/rlimit_other_test.go index 8c05375..f830ee1 100644 --- a/rlimit_other_test.go +++ b/rlimit_other_test.go @@ -2,14 +2,15 @@ // Use of this source code is governed by a BSD-3-Clause license that can be // found in the LICENSE file at the root of this repository. -//go:build !unix +//go:build !linux package pdfkit import "errors" -// capAddressSpace has no counterpart outside the Unixes, so the bomb test -// skips rather than running without the limit that makes it safe. +// capAddressSpace has no dependable counterpart off Linux — macOS rejects +// RLIMIT_AS with EINVAL — so the bomb test skips rather than running without +// the limit that makes it safe to run at all. func capAddressSpace(uint64) error { - return errors.New("no RLIMIT_AS on this platform") + return errors.New("no dependable RLIMIT_AS on this platform") } diff --git a/rlimit_unix_test.go b/rlimit_unix_test.go deleted file mode 100644 index ce63c41..0000000 --- a/rlimit_unix_test.go +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (c) 2026 the go-pdfkit/pdfkit authors. All rights reserved. -// Use of this source code is governed by a BSD-3-Clause license that can be -// found in the LICENSE file at the root of this repository. - -//go:build unix - -package pdfkit - -import "syscall" - -// capAddressSpace lowers this process's address-space ceiling to n bytes, so a -// decode that asks for more dies here instead of on the machine running the -// tests. It never raises the ceiling: a limit already tighter than n is left -// alone, because something else set it for a reason. -func capAddressSpace(n uint64) error { - var cur syscall.Rlimit - if err := syscall.Getrlimit(syscall.RLIMIT_AS, &cur); err != nil { - return err - } - if cur.Cur != 0 && cur.Cur < n { - return nil - } - lim := syscall.Rlimit{Cur: n, Max: cur.Max} - if cur.Max != 0 && cur.Max < n { - lim.Cur = cur.Max - } - return syscall.Setrlimit(syscall.RLIMIT_AS, &lim) -} From 7cd876ba100a208dd125f1f6bf7ed7d9ed106982 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 4 Nov 2026 17:12:52 +0100 Subject: [PATCH 3/4] fix: take the default from render.go, not from a neighbour's sentence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⛔⛔ I wrote "the same ceiling go-pdfkit/render uses" in the doc comment, in the README and in the pull request, and had not opened render.go. It says const defaultMaxPixels = 40 << 20 (render.go:79) which is 41 943 040, not the 40 000 000 I had copied from go-gfx/gfx's svg package. A difference of 1 943 040 pixels, asserted three times as an equality. gfx says it too, in two places -- svg/svg.go:79 and README.md:46 -- and codec/codec.go:201 carries the same constant. So three repositories were asserting a parity none of them had opened the file to confirm, and pdfkit was about to be the fourth. Filed as go-gfx/gfx#92. DefaultMaxPixels is now 40 << 20: render's number, read there, with the file and line in the comment. It is also the one whose arithmetic is exact -- four bytes a pixel is 160 MiB on the nose, where 40 000 000 is 152.6 MiB described as "a hundred and sixty million bytes". Neither number was more correct than the other. What was wrong was a statement of fact with nothing behind it, which is the kind of thing that travels. The change-detector test now also fails if the default drifts back onto 40_000_000, and says why. Co-Authored-By: Claude Opus 5 --- README.md | 19 +++++++++++++------ ceiling_test.go | 14 +++++++++++--- document.go | 16 +++++++++++----- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index bc3a96d..094d34d 100644 --- a/README.md +++ b/README.md @@ -86,12 +86,19 @@ decoder returns is not a ceiling, it is a post-mortem. The spec asks for the same reading order: "PNG decoders should detect errors as early as possible" (§4.10). -`Options.MaxPixels` defaults to `DefaultMaxPixels`, **forty million pixels** — -a hundred and sixty million bytes, 152 MiB, at the four bytes a pixel -`image/png` hands back. That is the ceiling `go-pdfkit/render` and -`go-gfx/gfx`'s `svg` package already use, and a little over A4 at 600 dpi. A -program embedding its own logo never meets it; a program embedding pictures it -did not produce should set it to what its own documents need. +`Options.MaxPixels` defaults to `DefaultMaxPixels` = `40 << 20`, **41 943 040 +pixels** — exactly 160 MiB at the four bytes a pixel `image/png` hands back, +and a little over A4 at 600 dpi. A program embedding its own logo never meets +it; a program embedding pictures it did not produce should set it to what its +own documents need. + +⛔ That number is `go-pdfkit/render`'s `defaultMaxPixels` (`render.go:79`), +read there rather than taken on trust. `go-gfx/gfx`'s `svg.DefaultMaxPixels` is +`40_000_000` and its documentation calls that *"the same ceiling +go-pdfkit/render uses"* — it is not, and the two differ by 1 943 040 pixels. +That sentence had been travelling between repositories unchecked, this README +included, until someone opened `render.go` +([go-gfx/gfx#92](https://github.com/go-gfx/gfx/issues/92)). Two neighbours are deliberately **not** bounded, and the reasons are worth stating because "it has a limit" is otherwise read as "everything has a limit": diff --git a/ceiling_test.go b/ceiling_test.go index 0fd8f09..fbd3955 100644 --- a/ceiling_test.go +++ b/ceiling_test.go @@ -152,12 +152,20 @@ func TestTheDefaultCeilingIsTheNumberTheDocumentationQuotes(t *testing.T) { // Written out rather than derived from the constant: deriving it would // make the judge move with the subject, which is how a ceiling test passes // a mutation that raised the ceiling. - if DefaultMaxPixels != 40_000_000 { + if DefaultMaxPixels != 41_943_040 { t.Errorf("DefaultMaxPixels is %d. If that was meant, update Options.MaxPixels's "+ "doc comment, DefaultMaxPixels's, and the README.", DefaultMaxPixels) } - if got := fmt.Sprintf("%d MiB", 40_000_000*4/(1<<20)); got != "152 MiB" { - t.Errorf("forty million pixels at four bytes each is %s", got) + if got := fmt.Sprintf("%d MiB", 41_943_040*4/(1<<20)); got != "160 MiB" { + t.Errorf("the default at four bytes a pixel is %s", got) + } + // ⛔ And the sentence that sent me to check. go-gfx/gfx's svg package + // documents 40_000_000 as "the same ceiling go-pdfkit/render uses"; it is + // not, render.go:79 is 40 << 20. Written out so that aligning on a figure + // borrowed from a neighbour's prose cannot happen here quietly. + if DefaultMaxPixels == 40_000_000 { + t.Error("the default has drifted onto go-gfx/gfx's number, which is the one that " + + "claims a parity with go-pdfkit/render that it does not have") } } diff --git a/document.go b/document.go index 10234c9..120ef89 100644 --- a/document.go +++ b/document.go @@ -93,11 +93,17 @@ const DefaultProducer = "go-pdfkit/pdfkit" // [Options.MaxPixels] is zero. // // Said in bytes, because a count of pixels does not tell anybody what it -// permits: image/png hands back four bytes a pixel, so forty million pixels is -// a hundred and sixty MILLION BYTES — 152 MiB — before pdfkit has copied a -// single sample. That is the ceiling go-pdfkit/render and go-gfx/gfx's svg -// package already use, and it is a little over A4 at 600 dots to the inch. -const DefaultMaxPixels = 40_000_000 +// permits: image/png hands back four bytes a pixel, so this is 160 MiB held +// before pdfkit has copied a single sample. It is a little over A4 at 600 dots +// to the inch. +// +// ⛔ The number is go-pdfkit/render's defaultMaxPixels (render.go:79), read +// there rather than taken on trust. go-gfx/gfx's svg.DefaultMaxPixels is +// 40_000_000 and its documentation calls that "the same ceiling +// go-pdfkit/render uses"; the two differ by 1 943 040 pixels, and that +// sentence has been travelling between repositories unchecked. Filed as +// go-gfx/gfx#92. +const DefaultMaxPixels = 40 << 20 // maxPixels is the ceiling in force for this document. func (d *Document) maxPixels() int { From 1fecd4841124f6724a15886e0ab71f07406f4ffe Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 4 Nov 2026 17:15:21 +0100 Subject: [PATCH 4/4] test: a skip of the bomb test on Linux is a failure, not a skip CI's first green run of this test proved less than it looked like. Every step passed, including "Test with coverage" -- but CI runs go test without -v, and a SKIP and a PASS are the same colour there. If capAddressSpace had returned an error on the runner, the child would have skipped, the parent would have skipped, and the lane would have been green over a bomb test that never fired a single byte at anything. On Linux there is no excuse: /proc/self/statm and RLIMIT_AS are both there. So a skip on Linux is now a FAILURE that says the test has stopped testing anything, and the only platforms permitted to skip are the ones the message names. A pass on Linux now means both halves ran: the guarded child refused the bomb and exited cleanly, AND the bare control died under the same headroom. Neither alone would be worth a green light. Co-Authored-By: Claude Opus 5 --- ceiling_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ceiling_test.go b/ceiling_test.go index fbd3955..b91b097 100644 --- a/ceiling_test.go +++ b/ceiling_test.go @@ -220,6 +220,15 @@ func runBombChild(t *testing.T, mode string) ([]byte, error) { cmd.Env = append(os.Environ(), bombEnv+"="+mode, "GOMEMLIMIT=256MiB") out, err := cmd.CombinedOutput() if bytes.Contains(out, []byte(skipMark)) { + // ⛔⛔ A skip and a pass are the same colour, and CI does not run with + // -v, so a limit that silently stopped working would look exactly like + // this test doing its job. On Linux there is no excuse — /proc and + // RLIMIT_AS are both there — so a skip there is a FAILURE, and the + // only platforms that may skip are the ones named in the message. + if runtime.GOOS == "linux" { + t.Fatalf("the child could not take an address-space limit on linux, where it "+ + "should always be able to; this test has stopped testing anything:\n%s", out) + } t.Skipf("%s does not enforce an address-space limit, so the bomb is left to the "+ "heap measurement in TestAPNGIsRefusedFromItsHeaderBeforeItsPixelsAreAllocated", runtime.GOOS)