From 539a1c16cd4e11feb49511e1b8b859c03d478076 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 16:08:33 +0200 Subject: [PATCH] Refuse an xz file of more than 1,024 blocks before decoding it (review 68, hq issue 345) The xz reader keeps one delta or BCJ decoder for every block of a stream and frees them only when the stream ends, so a file of many tiny blocks grows the heap with its length: about 67 MiB live for a 6 MB `xz --block-size=1 --delta` file, about 5.6 GiB at the 512 MiB download cap. The file's framing is now walked first, without decompressing: stream header, each block header, the LZMA2 chunk headers (each carries its compressed size, so a chunk is skipped unread), padding, check, index and footer. A file of more than 1,024 blocks or 16 streams is refused, as is an index whose block count is not the count walked. The index alone is not trusted because the reader compares it with the blocks only after decoding them all, so a lying index would still be decoded. Refusing delta and BCJ outright was not chosen: it needs the same walk to see each block header, and would refuse sound releases filtered with BCJ. 1,024 blocks keep at most about 0.35 MiB of decoders. Betterbird's archive is 1 stream with 1 block; multi-threaded xz cuts blocks of 3 times the dictionary, so 512 MiB in blocks of 512 KiB still fits. The library stays unforked. --- internal/builder/upstream_file.go | 7 +- internal/builder/xz_framing.go | 290 ++++++++++++++++++++++++++++ internal/builder/xz_framing_test.go | 207 ++++++++++++++++++++ 3 files changed, 503 insertions(+), 1 deletion(-) create mode 100644 internal/builder/xz_framing.go create mode 100644 internal/builder/xz_framing_test.go diff --git a/internal/builder/upstream_file.go b/internal/builder/upstream_file.go index 38d374e7..c4d3a730 100644 --- a/internal/builder/upstream_file.go +++ b/internal/builder/upstream_file.go @@ -198,7 +198,12 @@ func unpackUpstream(ctx context.Context, file, into, within string) error { // may not carry: the seat's image is the catalogue's build-agent recipe, which installs git and docker-cli // alone, and a release pinned as .tar.xz failed there on 2026-10-12 (hq issue 345). xzDictMax is a real // maximum: a block whose LZMA2 dictionary is larger is refused before its dictionary is allocated - // (ErrMemlimit). What the stream unpacks to is bounded by untar (maxUpstream, maxEntries). + // (ErrMemlimit). The reader keeps a filter decoder for every block of a stream, so the file's framing is + // walked first and a file of more blocks than xzBlocksMax is refused before anything is decoded + // (xz_framing.go). What the stream unpacks to is bounded by untar (maxUpstream, maxEntries). + if err := xzWithinLimits(file); err != nil { + return err + } zipped, err := xz.NewReader(buffered, xzDictMax) if err != nil { return fmt.Errorf("it is not an xz stream this build seat reads (a dictionary of %d MiB at most): %v", diff --git a/internal/builder/xz_framing.go b/internal/builder/xz_framing.go new file mode 100644 index 00000000..d750c1ef --- /dev/null +++ b/internal/builder/xz_framing.go @@ -0,0 +1,290 @@ +package builder + +import ( + "bufio" + "bytes" + "encoding/binary" + "errors" + "fmt" + "hash/crc32" + "io" + "os" +) + +// xzBlocksMax and xzStreamsMax are the most blocks and the most concatenated streams an xz file may hold for +// the build seat to read it. +// +// **Why a block count is limited at all.** The xz reader (github.com/therootcompany/xz v1.0.1) keeps one +// filter decoder for every block that names a delta or BCJ filter, and frees them only when a stream ends: +// its block counters are reset per stream, not per block. A stream of many tiny blocks therefore grows the +// heap with its length: an `xz --block-size=1 --delta` file of 6 MB kept about 67 MiB live, which comes to +// about 5.6 GiB at the 512 MiB download cap (review 68 on mesh-controller pull request 264, hq issue 345). +// +// **Why 1,024.** At about 340 bytes of decoder a block, 1,024 blocks keep at most about 0.35 MiB live. A +// real release is far below it: Betterbird's archive is 1 stream with 1 block, and multi-threaded xz cuts +// blocks of three times the dictionary (24 MiB at the default preset), so even 512 MiB unpacked in blocks +// of 512 KiB stays within it. Streams are limited to 16 because a release is one stream; the decoders are +// reused from one stream to the next, so the stream limit only bounds the work of this walk. +const ( + xzBlocksMax = 1024 + xzStreamsMax = 16 +) + +// xzWithinLimits walks an xz file's framing, without decompressing anything, and refuses one holding more +// blocks or streams than the build seat reads. +// +// **The framing, not the index.** Each stream's index at its end lists its blocks, but the reader compares +// the index with the blocks only after it has decoded them all, so a file whose index lies would still be +// decoded block by block. This walk follows the same framing the reader follows: stream header, each block +// header, the block's LZMA2 chunks (each chunk header carries its own compressed size, so a chunk is skipped +// without being decoded), its padding and check, then the index and the footer. It counts the blocks the +// reader would decode, and it also refuses an index whose count of blocks is not the count walked. +func xzWithinLimits(file string) error { + f, err := os.Open(file) + if err != nil { + return err + } + defer f.Close() + _, _, err = xzFraming(bufio.NewReader(f)) + if errors.Is(err, errXzTooManyBlocks) || errors.Is(err, errXzTooManyStreams) { + return err + } + if err != nil { + return fmt.Errorf("it is not an xz file this build seat reads: %v", err) + } + return nil +} + +var ( + errXzTooManyBlocks = fmt.Errorf("it holds more than %d xz blocks, the most this build seat reads: "+ + "its xz reader keeps a filter decoder for every block (hq issue 345)", xzBlocksMax) + errXzTooManyStreams = fmt.Errorf("it holds more than %d concatenated xz streams, the most this build "+ + "seat reads", xzStreamsMax) +) + +// xzFraming counts the streams and blocks of an xz file, checking its framing as it goes. It stops with +// errXzTooManyBlocks or errXzTooManyStreams at the first block or stream over the limit. +func xzFraming(r *bufio.Reader) (streams, blocks int, err error) { + full := func(n int) ([]byte, error) { + b := make([]byte, n) + if _, err := io.ReadFull(r, b); err != nil { + if err == io.EOF { + err = io.ErrUnexpectedEOF + } + return nil, err + } + return b, nil + } + skip := func(n int) error { + if got, err := r.Discard(n); err != nil || got != n { + return io.ErrUnexpectedEOF + } + return nil + } + for { + header, err := full(12) + if err != nil { + return streams, blocks, fmt.Errorf("stream %d: header: %v", streams+1, err) + } + if !bytes.Equal(header[:6], []byte{0xfd, '7', 'z', 'X', 'Z', 0x00}) { + return streams, blocks, fmt.Errorf("stream %d: no xz stream header", streams+1) + } + if header[6] != 0 || header[7]&0xf0 != 0 || + crc32.ChecksumIEEE(header[6:8]) != binary.LittleEndian.Uint32(header[8:12]) { + return streams, blocks, fmt.Errorf("stream %d: a corrupt stream header", streams+1) + } + if streams++; streams > xzStreamsMax { + return streams, blocks, errXzTooManyStreams + } + checkSize := 0 + if check := int(header[7]); check != 0 { + checkSize = 4 << ((check - 1) / 3) + } + inStream := 0 + for { + first, err := r.ReadByte() + if err != nil { + return streams, blocks, fmt.Errorf("stream %d: no index: %v", streams, io.ErrUnexpectedEOF) + } + if first == 0 { + break // the index indicator + } + rest, err := full(int(first)*4 + 3) + if err != nil { + return streams, blocks, fmt.Errorf("stream %d, block %d: header: %v", streams, inStream+1, err) + } + if err := xzBlockHeader(append([]byte{first}, rest...)); err != nil { + return streams, blocks, fmt.Errorf("stream %d, block %d: %v", streams, inStream+1, err) + } + inStream++ + if blocks++; blocks > xzBlocksMax { + return streams, blocks, errXzTooManyBlocks + } + // The block's LZMA2 chunks, up to the end marker. + size := 0 + for { + control, err := r.ReadByte() + if err != nil { + return streams, blocks, fmt.Errorf("stream %d, block %d: %v", streams, inStream, io.ErrUnexpectedEOF) + } + size++ + if control == 0 { + break + } + switch { + case control == 1 || control == 2: // stored: a 16-bit size less one, then the bytes + b, err := full(2) + if err != nil { + return streams, blocks, err + } + n := int(binary.BigEndian.Uint16(b)) + 1 + if err := skip(n); err != nil { + return streams, blocks, err + } + size += 2 + n + case control >= 0x80: // LZMA: unpacked and packed sizes, the properties byte on a reset + b, err := full(4) + if err != nil { + return streams, blocks, err + } + n := int(binary.BigEndian.Uint16(b[2:])) + 1 + size += 4 + if control >= 0xc0 { + if err := skip(1); err != nil { + return streams, blocks, err + } + size++ + } + if err := skip(n); err != nil { + return streams, blocks, err + } + size += n + default: + return streams, blocks, fmt.Errorf("stream %d, block %d: an LZMA2 chunk of kind %#x", + streams, inStream, control) + } + } + if pad := (4 - size%4) % 4; pad > 0 { + b, err := full(pad) + if err != nil || !bytes.Equal(b, make([]byte, pad)) { + return streams, blocks, fmt.Errorf("stream %d, block %d: corrupt block padding", streams, inStream) + } + } + if err := skip(checkSize); err != nil { + return streams, blocks, err + } + } + // The index: its count of records, two numbers each, padding, CRC32. + used := 1 + count, n, err := xzVLI(r) + if err != nil { + return streams, blocks, fmt.Errorf("stream %d: index: %v", streams, err) + } + used += n + if count != uint64(inStream) { + return streams, blocks, fmt.Errorf("stream %d: its index lists %d blocks, but it holds %d", + streams, count, inStream) + } + for i := 0; i < 2*inStream; i++ { + _, n, err := xzVLI(r) + if err != nil { + return streams, blocks, fmt.Errorf("stream %d: index: %v", streams, err) + } + used += n + } + if err := skip((4-used%4)%4 + 4); err != nil { + return streams, blocks, fmt.Errorf("stream %d: index: %v", streams, err) + } + footer, err := full(12) + if err != nil || footer[10] != 'Y' || footer[11] != 'Z' { + return streams, blocks, fmt.Errorf("stream %d: no stream footer", streams) + } + // Stream padding: zero bytes, a multiple of four, then the end or another stream. + zeros := 0 + for { + b, err := r.ReadByte() + if err == io.EOF { + if zeros%4 != 0 { + return streams, blocks, errors.New("stream padding not a multiple of four bytes") + } + return streams, blocks, nil + } + if err != nil { + return streams, blocks, err + } + if b != 0 { + _ = r.UnreadByte() + break + } + zeros++ + } + if zeros%4 != 0 { + return streams, blocks, errors.New("stream padding not a multiple of four bytes") + } + } +} + +// xzBlockHeader checks one block header: its CRC32, its flags, and that its last filter is LZMA2, the only +// one whose chunks this walk can skip (and the only last filter the reader decodes). +func xzBlockHeader(h []byte) error { + size := len(h) + if crc32.ChecksumIEEE(h[:size-4]) != binary.LittleEndian.Uint32(h[size-4:]) { + return errors.New("a corrupt block header") + } + flags := h[1] + if flags&0x3c != 0 { + return errors.New("a block header with reserved flags") + } + body := bytes.NewReader(h[2 : size-4]) + if flags&0x40 != 0 { + if _, _, err := xzVLI(body); err != nil { + return err + } + } + if flags&0x80 != 0 { + if _, _, err := xzVLI(body); err != nil { + return err + } + } + var last uint64 + for i := 0; i <= int(flags&3); i++ { + id, _, err := xzVLI(body) + if err != nil { + return err + } + props, _, err := xzVLI(body) + if err != nil { + return err + } + if props > uint64(body.Len()) { + return errors.New("a corrupt block header") + } + if _, err := body.Seek(int64(props), io.SeekCurrent); err != nil { + return err + } + last = id + } + if last != 0x21 { + return fmt.Errorf("a block whose last filter is %#x, not LZMA2", last) + } + return nil +} + +// xzVLI reads one of xz's variable-length integers (at most 9 bytes, 7 bits each), and how many bytes it took. +func xzVLI(r io.ByteReader) (uint64, int, error) { + var v uint64 + for i := 0; i < 9; i++ { + b, err := r.ReadByte() + if err != nil { + return 0, i, io.ErrUnexpectedEOF + } + v |= uint64(b&0x7f) << (7 * i) + if b&0x80 == 0 { + if i > 0 && b == 0 { + return 0, i + 1, errors.New("a number not in its shortest form") + } + return v, i + 1, nil + } + } + return 0, 9, errors.New("a number longer than nine bytes") +} diff --git a/internal/builder/xz_framing_test.go b/internal/builder/xz_framing_test.go new file mode 100644 index 00000000..a672cb80 --- /dev/null +++ b/internal/builder/xz_framing_test.go @@ -0,0 +1,207 @@ +package builder + +import ( + "archive/tar" + "bufio" + "bytes" + "context" + "encoding/binary" + "errors" + "hash/crc32" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// xzStream is one xz stream (no integrity check) of data cut into blocks of blockSize bytes, each block's +// LZMA2 data stored uncompressed, each block a delta filter before LZMA2 when delta is set: the shape of +// `xz --block-size=N --filters=delta,lzma2`, built without any xz tool. listed is how many blocks its index +// lists (-1: as many as it holds), so a stream whose index lies can be built too. +func xzStream(data []byte, blockSize int, delta bool, listed int) []byte { + var out bytes.Buffer + flags := []byte{0, 0} + out.Write([]byte{0xfd, '7', 'z', 'X', 'Z', 0x00}) + out.Write(flags) + out.Write(binary.LittleEndian.AppendUint32(nil, crc32.ChecksumIEEE(flags))) + type record struct{ unpadded, uncompressed uint64 } + var records []record + for at := 0; at < len(data); at += blockSize { + block := data[at:min(at+blockSize, len(data))] + if delta { + // Delta-encoded with a distance of one; the decoder's history starts empty at each block. + encoded := make([]byte, len(block)) + for i := range block { + encoded[i] = block[i] + if i > 0 { + encoded[i] -= block[i-1] + } + } + block = encoded + } + header := []byte{2, 0} // 12 bytes + if delta { + header[1] = 1 // two filters + header = append(header, 0x03, 0x01, 0x00) + } + header = append(header, 0x21, 0x01, 0x00) // LZMA2, a 4 KiB dictionary + for len(header) < 8 { + header = append(header, 0) + } + header = binary.LittleEndian.AppendUint32(header, crc32.ChecksumIEEE(header)) + out.Write(header) + size := 0 + for i, chunk := 0, block; len(chunk) > 0; i++ { + n := min(len(chunk), 1<<16) + control := byte(2) + if i == 0 { + control = 1 // the first chunk of a block resets the dictionary + } + out.WriteByte(control) + out.Write(binary.BigEndian.AppendUint16(nil, uint16(n-1))) + out.Write(chunk[:n]) + size += 3 + n + chunk = chunk[n:] + } + out.WriteByte(0) + size++ + out.Write(make([]byte, (4-size%4)%4)) + records = append(records, record{uint64(len(header) + size), uint64(len(block))}) + } + if listed >= 0 { + for len(records) < listed { + records = append(records, records[0]) + } + records = records[:listed] + } + vli := func(b []byte, v uint64) []byte { + for v >= 0x80 { + b = append(b, byte(v)|0x80) + v >>= 7 + } + return append(b, byte(v)) + } + index := vli([]byte{0}, uint64(len(records))) + for _, r := range records { + index = vli(vli(index, r.unpadded), r.uncompressed) + } + for len(index)%4 != 0 { + index = append(index, 0) + } + index = binary.LittleEndian.AppendUint32(index, crc32.ChecksumIEEE(index)) + out.Write(index) + footer := binary.LittleEndian.AppendUint32(nil, uint32(len(index)/4-1)) + footer = append(footer, flags...) + out.Write(binary.LittleEndian.AppendUint32(nil, crc32.ChecksumIEEE(footer))) + out.Write(footer) + out.WriteString("YZ") + return out.Bytes() +} + +// tarHolding is a tar holding betterbird/data with body. +func tarHolding(t *testing.T, body []byte) []byte { + t.Helper() + var out bytes.Buffer + w := tar.NewWriter(&out) + if err := w.WriteHeader(&tar.Header{Name: "betterbird/data", Mode: 0o644, Size: int64(len(body)), + Typeflag: tar.TypeReg}); err != nil { + t.Fatal(err) + } + w.Write(body) + if err := w.Close(); err != nil { + t.Fatal(err) + } + return out.Bytes() +} + +func writeTemp(t *testing.T, body []byte) string { + t.Helper() + file := filepath.Join(t.TempDir(), "r.tar.xz") + if err := os.WriteFile(file, body, 0o644); err != nil { + t.Fatal(err) + } + return file +} + +func TestAnXzFileOfAFewDeltaBlocksAndTwoStreamsIsRead(t *testing.T) { + // The streams this test builds are ones the reader decodes, so what the walk refuses below is the + // same framing the reader follows. + body := bytes.Repeat([]byte("betterbird "), 300) + whole := tarHolding(t, body) + half := len(whole) / 2 + for name, file := range map[string][]byte{ + "one stream of delta blocks": xzStream(whole, 512, true, -1), + "two streams with padding": append(append(xzStream(whole[:half], 700, true, -1), 0, 0, 0, 0), + xzStream(whole[half:], 100, false, -1)...), + } { + path := writeTemp(t, file) + if xzPath, err := exec.LookPath("xz"); err == nil { + if out, err := exec.Command(xzPath, "-t", path).CombinedOutput(); err != nil { + t.Fatalf("%s: xz itself refuses the built stream: %v %s", name, err, out) + } + } + into := t.TempDir() + if err := unpackUpstream(context.Background(), path, into, "betterbird"); err != nil { + t.Fatalf("%s: %v", name, err) + } + got, err := os.ReadFile(filepath.Join(into, "data")) + if err != nil || !bytes.Equal(got, body) { + t.Fatalf("%s: unpacked %d bytes, want %d: %v", name, len(got), len(body), err) + } + } +} + +func TestAnXzFileOfManyDeltaBlocksIsRefusedBeforeItIsDecoded(t *testing.T) { + // Review 68 on pull request 264: `xz --block-size=1 --delta` of a 200 kB tar kept about 67 MiB live in + // the reader, one delta decoder per block. Its index here lists one block, as a file crafted to pass a + // check of the index alone would; the walk counts the blocks the reader would decode. + body := make([]byte, 200_000) + for i := range body { + body[i] = byte(i * 7) + } + crafted := xzStream(tarHolding(t, body), 1, true, 1) + path := writeTemp(t, crafted) + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + err := unpackUpstream(context.Background(), path, t.TempDir(), "betterbird") + runtime.ReadMemStats(&after) + if err == nil || !strings.Contains(err.Error(), "more than 1024 xz blocks") { + t.Fatalf("a file of %d bytes in blocks of one byte was not refused for its blocks: %v", len(crafted), err) + } + if grew := after.TotalAlloc - before.TotalAlloc; grew > 4<<20 { + t.Fatalf("%d bytes were allocated for a refused file", grew) + } +} + +func TestTheXzBlockLimitIsExact(t *testing.T) { + data := make([]byte, xzBlocksMax+1) + streams, blocks, err := xzFraming(bufio.NewReader(bytes.NewReader(xzStream(data[:xzBlocksMax], 1, true, -1)))) + if err != nil || streams != 1 || blocks != xzBlocksMax { + t.Fatalf("%d blocks: %d streams, %d blocks, %v", xzBlocksMax, streams, blocks, err) + } + if _, _, err := xzFraming(bufio.NewReader(bytes.NewReader(xzStream(data, 1, true, -1)))); !errors.Is(err, errXzTooManyBlocks) { + t.Fatalf("%d blocks: %v", xzBlocksMax+1, err) + } +} + +func TestAnXzFileOfTooManyStreamsIsRefused(t *testing.T) { + var file []byte + for i := 0; i <= xzStreamsMax; i++ { + file = append(file, xzStream([]byte("x"), 1, false, -1)...) + } + err := unpackUpstream(context.Background(), writeTemp(t, file), t.TempDir(), "") + if err == nil || !strings.Contains(err.Error(), "more than 16 concatenated xz streams") { + t.Fatalf("%v", err) + } +} + +func TestAnXzIndexThatDoesNotListItsBlocksIsRefused(t *testing.T) { + file := xzStream(tarHolding(t, []byte("x")), 512, false, 1) + err := unpackUpstream(context.Background(), writeTemp(t, file), t.TempDir(), "betterbird") + if err == nil || !strings.Contains(err.Error(), "its index lists 1 blocks, but it holds 4") { + t.Fatalf("%v", err) + } +}