Conversation
The parallel reader allocates a whole block up front from the uncompressed size recorded in the index, before decoding anything. That number was only checked for being non-negative, so a 44-byte file claiming 2^49 bytes aborted the process with "makeslice: len out of range" — and because the allocation happens on a worker goroutine, the caller could not recover from it. Bound each record's uncompressed size by what a block of its unpadded size could actually produce, and reject sizes beyond the address space so 32-bit builds do not truncate instead. As defence in depth, convert a panic escaping a worker into a decode error rather than letting it kill the process. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
The parallel reader parses the index before the blocks, so it cannot cross-check the declared record count and passes -1 to skip that check. That left the count — a raw uvarint from the file — sizing make([]record, n) directly, so a 40-byte file reserved 16 TiB and a 44-byte one panicked in makeslice. TODO.md records this exact bug being found by fuzzing in 2021 and fixed by adding the count check; disabling the check for the parallel path reopened it. Grow the slice with the records that actually arrive. A hostile count now runs into the end of the index and returns EOF, and the check stays in place for the sequential reader that can still afford it. Also make the hostile-file tests assert an allocation budget, since returning an error is not enough if the heap is already gone. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
parseBlocks summed the padded block sizes from the index with no overflow check. Two records of 2^63-1 wrap the sum negative, which puts the computed stream header position at or after the footer the walk started from, so pos never decreases and a 104-byte file spins NewParallelReader at 100% CPU forever. It takes no context, so there is no way to cancel it. Accumulate against the space actually available between the stream header and the index: every partial sum stays in [0, indexStart], so neither the comparison nor the addition can overflow. Add an explicit check that each stream precedes the index it was found from, so the loop's termination is checkable locally. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
The dictionary capacity in an xz block header is attacker controlled and reaches 4 GiB, and it was allocated in full before decoding started. A 104-byte file with the dictionary-size property set to the maximum forced a 4 GiB allocation and then decoded 45 bytes. ReaderConfig.DictCap could not prevent it: the filter's value replaces the caller's whenever it is larger, so it is a floor, not a ceiling. The lzma package already treats its own DictCap as an upper limit for .lzma files; the .xz path never got the same treatment. Start the dictionary at 64 KiB and double towards the declared capacity as data arrives, so it costs what the stream produces rather than what its header claims. Growth happens before the ring buffer would first wrap, so relocating it is a prefix copy and no index changes; wrapping only becomes possible once the declared capacity is reached, which is where the old behaviour resumes exactly. The bomb file now allocates 0.1 MiB instead of 4 GiB. Reader throughput is unchanged; doubling costs about twice the final dictionary in transient garbage. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
A properties byte can ask for lc=8 lp=4, which sizes the literal codec at 0x300<<12 probabilities: 6 MiB allocated and refilled by every LZMA2 chunk that carries a property reset. 480 KB of such chunks drove 120 GiB of allocation and 16 seconds of CPU while producing only 40 MB of output, so output-size bomb detection does not see it. The reference implementation rejects lc+lp > 4 when decoding the properties byte of both LZMA and LZMA2 streams, and our own writer has always refused to emit it; only the decoder let it through. Apply the rule in Properties.verify so both directions share one definition, and drop the writer's separate copy. Two header test fixtures used lc=4 lp=3, a combination no real file can contain; they now use lc=2 lp=2. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Close is documented as the way to abandon a ParallelReader, but a Read waiting on a block never noticed it. The cancelled dispatcher closed jobs and not queue, and could leave a block queued that no worker would ever report on, so both of nextBlock's receives could park forever. Close from another goroutine also raced with start over done, and with Read over err; -race reported both. Close both channels on every dispatcher exit path, watch done in nextBlock, create done with the reader so Close cannot race a first Read, and put err behind a mutex. err now keeps the first failure and returns it consistently, so a reader that reached io.EOF still reports io.EOF after a later Close. Read and WriteTo remain single-goroutine; Close is now genuinely safe from another one, and the type documentation says so instead of leaving it to "not safe for concurrent use". Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Correctness: checked accumulation for the index size total so Size cannot wrap; a finalizer so an abandoned ParallelReader does not leak its workers for the life of the process; io.ErrShortWrite instead of an infinite retry when a writer accepts nothing and reports no error; re-apply the Workers floor in start, since it is a promoted field and zero workers left every read waiting; a length guard on the block header. API: WriteTo on a drained reader returns success rather than io.EOF, which io.Copy would surface as a failure. ReaderConfig.Verify and ParallelReaderConfig.Verify now write back the defaults they always claimed to apply, and DictCap is documented as the floor it is. New ErrCorrupt, ErrClosed and ErrUnsupported let callers separate a bad file from a bad transport with errors.Is instead of matching message text; the messages themselves are unchanged, and an I/O error from the underlying reader still passes through unwrapped. Added interface assertions. Security: scan stream padding in blocks rather than one four-byte read per group, so a zero-padded file is not millions of syscalls. Reject non-canonical variable-length integers and cap them at the nine bytes the format allows, matching liblzma — accepting more meant reading files the reference decoder refuses. Docs: ignore .idea/, go install instead of the long-dead go get form, drop upstream text that predates this fork's parallel reader, correct the speedup claim, and fix typos. One test fixture round-tripped 1<<64-1 through the uvarint encoder, which needs ten bytes; the format's maximum is 1<<63-1 in nine. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
An LZMA2 chunk can reset the coder state, and state.Reset assigned a fresh struct value, dropping every codec's probability array. Each of the ~145 trees in the length, repeat length and distance codecs plus the literal codec then reallocated and refilled, and a chunk carrying new properties built a whole new state on top of that. A stream of small resetting chunks cost far more in allocator traffic than in decoding. Reuse the backing arrays when they are large enough, assign Reset's scalar fields individually instead of overwriting the struct, and reset in place in the reader. On 20000 resetting chunks: 385.3 MiB of allocation drops to 18.6 MiB and 95 ms to 67 ms. Decoded output is byte-identical and enwik7 is unaffected, having almost no resets. Reset no longer zeroes the struct, so a field added later without a matching reset line would survive one. TestStateResetReusesWithoutDrift dirties every probability and requires a reset state to deep-equal a fresh one. Also drop the per-byte modulo in both rolling hashes for an equivalent increment-and-compare. That one measures flat (p=0.74) because the writer is memory-stalled; it is kept as a simplification, not a win. PERF.md records both results, and corrects the long-standing claim that the writer's remaining cost is structural: profiling puts 75% in the match finder and 15% in the range encoder, and throughput tracks the match finder's working set rather than its call structure. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Adds five fuzz targets with a seed corpus, end-to-end coverage of all
four check types, and systematic malformed-input tests. Package
coverage goes from 77.6% to 87.6%.
The malformed-input sweep found a real bug. A file truncated at the
end of its last block, with the index and footer gone but every byte
of compressed data present, decoded to the full correct payload and
returned no error; xz -dc rejects the same bytes. streamReader.Read
read the bare io.EOF where the next block header belongs as "this
stream ended" rather than "this file was cut". Every xz stream ends
with an index and a footer, so that EOF is truncation. This is how an
interrupted download turns into silent data loss.
Coverage also showed newCRC32 was never called: the CRC-32 path, which
is what the reference tool emits by default, had no test at all. All
four checks now round-trip, detect a corrupted check, decode under
xz -dc, and read back files xz produced.
WriterConfig{CheckSum: None} silently yields CRC-64, since None is
zero and fill treats a zero CheckSum as unset. Left as is, because
changing it would alter existing callers' output; pinned by a test and
documented on the field.
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Applies the modernize analyzer (interface{} to any, range over int,
min, slices.Backward, obsolete +build lines), replaces the deprecated
io/ioutil, and moves the benchmarks to b.Loop, which also stops the
one-time setup in BenchmarkReader from polluting CPU profiles.
Adds .golangci.yml with 19 linters beyond the defaults, weighted
towards correctness: a codec's long decode loops and deliberate width
conversions fight style checks, so gosec's G115 and staticcheck's
QF1001 are off, each with its reason recorded in the file. All 87
issues it reported are fixed.
One of them was a bug. Writer2.Close discarded a failed Flush and
returned nil, so a write error while flushing the last chunk produced
a truncated stream and a successful Close. Together with the reader
accepting a file truncated after its last block, this package could
write a short archive silently and read it back without complaint.
Nothing in the suite had closed a writer over a failing sink.
Output is byte-identical, and an interleaved A/B shows the reader
slightly faster and the writer unchanged.
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Writer2.writeChunk picked the uncompressed chunk form whenever it was smaller, but that form is produced by copying the chunk's input back out of the encoder dictionary. On incompressible input a chunk grows to the 64 KiB compressed-data limit, and a dictionary smaller than that has already dropped part of the input, so CopyN came up short and Write or Close failed with "insufficient space". DictCap values down to MinDictCap (4096) are legal, which made those configurations unusable. Only choose the uncompressed form when the dictionary still holds the whole chunk — the same criterion CopyN truncates on — and otherwise write the compressed chunk, which always exists and always fits. Output is byte-identical for every configuration that worked before; the previously failing ones now pay at most the range-coder overhead per chunk instead of erroring. This also stops lzma.ErrNoSpace, which matches none of the package's sentinels, from escaping to callers. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Dropping a ParallelReader without Close left the dispatcher and every worker running for the life of the process: with more blocks than the queue holds, the dispatcher blocks on the full queue, never closes the job channel, and the workers never return. The SetFinalizer meant to cover this could not fire at all — the goroutines were started as methods on the reader, so their stacks kept it reachable, and the finalizer could only run once they had already exited on their own. Move everything the dispatcher and workers touch onto an internal parallelDecoder, so the goroutines no longer root the reader, and attach a runtime.AddCleanup to the reader that stops the decoder once the reader is collected. Close remains the documented way to release a reader early; the cleanup only keeps forgetting it from leaking. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
The parallel reader allocated each block's whole uncompressed size up front, straight from the index record. checkUncompressedSize bounds that declaration, but the surviving factor is about 350,000x the block's input, so a 5.4 KB file whose index declares 300 MiB for one 8 KiB block allocated 300 MiB on a worker goroutine before anything validated the claim — multiplied by every block in flight. Start each block at 1 MiB and grow the buffer geometrically with the bytes actually decoded, capped by the declared size, so memory tracks real output rather than the attacker-controlled declaration. Pooled buffers keep their capacity, so a warmed-up reader reslices instead of growing. The plausibility check stays: it rejects absurd records before a worker spends time on them and keeps Size meaningful. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Non-zero padding in a block header came back as errPadding, the sentinel the stream reader uses for stream-padding control flow. It is a plain errors.New, so the error did not match ErrCorrupt — breaking the contract that every corrupt-input error from this package matches it, and steering callers who use the sentinel to separate "reject the input" from "retry the transport" into retrying a permanently corrupt file — and its message talked about stream padding that is not involved. Return a corruptf error instead. Also replace the two surviving bare io.EOF comparisons in blockReader.Read with errors.Is, and document on Reader.Read that data returned alongside a corruption error can include trailing bytes decoded from input past the corruption point, an artifact of the range decoder's sticky error handling, and should be discarded. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Writer: the per-chunk state snapshot (cloneState) was 91% of the writer's allocations because the deepcopy methods always allocated fresh probability arrays. Let deepcopy reuse capacity like init does, snapshot into one persistent start state, and replace the state aliasing in writeUncompressedChunk with a copy. 4,624 -> 306 allocs/op; output is byte-identical. Reader: nothing was reused across blocks or chunks. Add lzma.Reader2.Reset and thread a cached reader through the xz filter plumbing — the sequential reader carries it across blocks and streams, each parallel worker owns one — so the decoder, its probability models and the dictionary survive block boundaries. Reinitialize the range decoder in place on Reopen and reuse the chunk header and its scratch buffer. 291 -> 132 allocs/op on one block; 16,859 -> 2,576 on a 153-block file. Parallel workers also keep one bufio.Reader, created on first use and sized to the largest block, instead of allocating 256 KiB per block: decoding 64 KiB-block files goes from 767 to 976 MB/s. Also guard the per-chunk/per-block xlog.Debugf calls behind a new xlog.DebugEnabled (the ...any boxing happens at the call site, before suppression), use stack arrays in the CRC Sum methods, replace the de Bruijn nlz32 with math/bits.LeadingZeros32, and document the intended cost of on-demand dictionary growth. LZMA1 input buffering (P9) is left alone deliberately: batching reads would consume input past the end of the stream and break embedded-stream callers. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.