AVRO-4287: [javascript] Enforce a maximum decompressed block size#3852
AVRO-4287: [javascript] Enforce a maximum decompressed block size#3852iemejia wants to merge 8 commits into
Conversation
When reading a data file, each block is decompressed according to the file's codec. A block with a very high compression ratio (or a malformed block) could expand to far more memory than its compressed size. Bound the decompressed size of each block: the built-in deflate codec passes the limit to zlib (via maxOutputLength) so the allocation itself is capped, and every codec's output is additionally checked as a safeguard. Mirrors the Java SDK's decompression limit (AVRO-4247). The limit defaults to 200 MiB and can be set per decoder via the maxDecompressLength option or globally with the AVRO_MAX_DECOMPRESS_LENGTH environment variable. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens the JavaScript Avro container-file BlockDecoder against decompression bombs by enforcing a configurable maximum decompressed block size (default 200 MiB, overridable via AVRO_MAX_DECOMPRESS_LENGTH or per-decoder maxDecompressLength), and adds tests to validate the behavior.
Changes:
- Add a default max decompressed block size with env-var override and a per-decoder option.
- Apply a decompressed-size safeguard to every codec output, and pass
maxOutputLengthto zlib for the built-indeflatecodec. - Add tests covering over-limit blocks, within-limit blocks, and custom codec enforcement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/js/lib/files.js | Introduces max decompression length configuration and enforces the limit during block decompression. |
| lang/js/test/test_files.js | Adds regression tests to ensure oversized blocks are rejected and custom codecs are also constrained. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this._codecs && codec === 'deflate') { | ||
| decompress = function (buf, cb) { | ||
| zlib.inflateRaw(buf, {maxOutputLength: maxLength}, cb); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fixed in a4a6680 — the zlib maxOutputLength cap is now applied whenever the resolved codec is zlib.inflateRaw, including a custom codecs map that reuses it. Added a test for that case.
| this._decompress = function (buf, cb) { | ||
| decompress(buf, function (err, data) { | ||
| if (err) { | ||
| cb(err); | ||
| return; | ||
| } | ||
| if (data && data.length > maxLength) { | ||
| cb(new Error(f( | ||
| 'decompressed block size exceeds the maximum allowed of %d bytes', | ||
| maxLength | ||
| ))); | ||
| return; | ||
| } | ||
| cb(null, data); | ||
| }); |
There was a problem hiding this comment.
Fixed in a4a6680 — _pending is now decremented in the block callback's error path, so a failed decompression (including the size safeguard firing) can no longer leave the stream waiting at finish.
…ing on error - Apply zlib's maxOutputLength cap whenever the resolved codec is zlib.inflateRaw, including a custom codecs map that reuses it (previously the cap was only applied for the built-in default, so custom maps fell back to the too-late post-decompress check). - Decrement _pending in the block callback's error path so a failed decompression (e.g. the size safeguard firing) cannot leave the stream waiting at finish. Assisted-by: GitHub Copilot:claude-opus-4.8
| if (err) { | ||
| self.emit('error', err); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fixed. On the error path in _createBlockCallback, if the writable side has already finished and this was the last pending block, the readable side is now ended (push(null)) so a consumer waiting on the queue is not left hanging after the error. Pushed in d1370e7.
| var encoder = new streams.BlockEncoder(t, {codec: 'deflate'}); | ||
| var decoder = new streams.BlockDecoder({maxDecompressLength: 1024}) | ||
| .on('data', function () {}) | ||
| .on('error', function () { cb(); }); |
There was a problem hiding this comment.
The test now asserts the specific error: zlib's ERR_BUFFER_TOO_LARGE from the maxOutputLength cap. Pushed in d1370e7.
| maxDecompressLength: 1024 | ||
| }) | ||
| .on('data', function () {}) | ||
| .on('error', function () { cb(); }); |
There was a problem hiding this comment.
Updated to assert err.code === 'ERR_BUFFER_TOO_LARGE', proving the maxOutputLength cap fired rather than any incidental error. Pushed in d1370e7.
| maxDecompressLength: 1024 | ||
| }) | ||
| .on('data', function () {}) | ||
| .on('error', function () { cb(); }); |
There was a problem hiding this comment.
Updated to assert the generic post-decompress safeguard message (/decompressed block size exceeds/) for the custom codec. Pushed in d1370e7.
There was a problem hiding this comment.
Already handled — the custom-codec test asserts the specific message /decompressed block size exceeds/ rather than passing on any error (test_files.js:611).
… specifics - In the block callback's error path, if the writable side already finished and this was the last pending block, end the readable side (push null) so a consumer waiting on the queue is not left hanging after the error. - The limit tests now assert the specific error: zlib's ERR_BUFFER_TOO_LARGE for the deflate maxOutputLength cap (built-in and custom-codec-reusing- inflateRaw), and the 'decompressed block size exceeds' message for the generic custom-codec safeguard. Assisted-by: GitHub Copilot:claude-opus-4.8
| function getDefaultMaxDecompressLength() { | ||
| var value = process.env.AVRO_MAX_DECOMPRESS_LENGTH; | ||
| if (value !== undefined) { | ||
| var parsed = parseInt(value, 10); | ||
| if (!isNaN(parsed) && parsed > 0) { | ||
| return parsed; | ||
| } | ||
| } | ||
| return DEFAULT_MAX_DECOMPRESS_LENGTH; | ||
| } |
There was a problem hiding this comment.
Fixed. AVRO_MAX_DECOMPRESS_LENGTH is now run through normalizeMaxDecompressLength, which yields a positive finite integer bounded by buffer.constants.MAX_LENGTH before it can reach zlib's maxOutputLength. Pushed in 3cba108.
| this._maxDecompressLength = opts.maxDecompressLength > 0 ? | ||
| opts.maxDecompressLength : getDefaultMaxDecompressLength(); |
There was a problem hiding this comment.
Fixed. opts.maxDecompressLength goes through the same normalizer as the env-var default (finite positive integer, clamped to the max buffer length). Added a test that an out-of-range limit is clamped rather than throwing. Pushed in 3cba108.
Both AVRO_MAX_DECOMPRESS_LENGTH and the maxDecompressLength option are now run through a single normalizer that yields a positive, finite integer bounded by the runtime's maximum buffer length. This prevents a very large, non-integer, or non-finite value from reaching zlib's maxOutputLength and throwing synchronously. Added a test that an out-of-range limit is clamped rather than crashing. Assisted-by: GitHub Copilot:claude-opus-4.8
| encoder.end(payload); | ||
| }); | ||
|
|
||
| it('clamps an out-of-range limit instead of throwing', function (cb) { |
There was a problem hiding this comment.
Fixed — renamed to 'falls back to the default limit for an out-of-range value' and reworded the comment, since normalizeMaxDecompressLength(Infinity) returns undefined and the decoder falls back to the default rather than clamping to the cap.
There was a problem hiding this comment.
Already handled — the test is now named 'falls back to the default limit for an out-of-range value' to match the fallback behavior.
…havior normalizeMaxDecompressLength(Infinity) returns undefined (non-finite), so the decoder falls back to the default limit rather than clamping to MAX_DECOMPRESS_LENGTH_CAP. Rename the test (and reword its comment) to describe the normalization/fallback it actually exercises. Assisted-by: GitHub Copilot:claude-opus-4.8
| it('falls back to the default limit for an out-of-range value', function (cb) { | ||
| // A non-finite/out-of-range limit is not usable, so normalizeMaxDecompressLength | ||
| // returns undefined and the decoder falls back to its default limit rather | ||
| // than making zlib throw synchronously; a normal block still decodes. | ||
| var t = createType('string'); | ||
| var payload = 'hello world'; | ||
| var out = []; | ||
| var encoder = new streams.BlockEncoder(t, {codec: 'deflate'}); | ||
| var decoder = new streams.BlockDecoder({maxDecompressLength: Infinity}) | ||
| .on('data', function (s) { out.push(s); }) | ||
| .on('end', function () { | ||
| assert.deepEqual(out, [payload]); | ||
| cb(); | ||
| }); | ||
| encoder.pipe(decoder); | ||
| encoder.end(payload); | ||
| }); |
There was a problem hiding this comment.
Fixed — the test now clears AVRO_MAX_DECOMPRESS_LENGTH (saving and restoring any prior value) so the fallback-to-default assertion is deterministic regardless of the runner environment.
There was a problem hiding this comment.
Already handled — the test saves, clears, and restores AVRO_MAX_DECOMPRESS_LENGTH so it's deterministic regardless of the runner environment.
The out-of-range fallback test asserts the decoder uses the library default, but the default can be overridden by AVRO_MAX_DECOMPRESS_LENGTH. Clear the env var for the duration of the test (restoring it afterward) so it is deterministic regardless of the runner environment. Assisted-by: GitHub Copilot:claude-opus-4.8
| var savedEnv = process.env.AVRO_MAX_DECOMPRESS_LENGTH; | ||
| delete process.env.AVRO_MAX_DECOMPRESS_LENGTH; | ||
| var restore = function () { | ||
| if (savedEnv === undefined) { | ||
| delete process.env.AVRO_MAX_DECOMPRESS_LENGTH; | ||
| } else { | ||
| process.env.AVRO_MAX_DECOMPRESS_LENGTH = savedEnv; | ||
| } | ||
| }; | ||
| var t = createType('string'); | ||
| var payload = 'hello world'; | ||
| var out = []; | ||
| var encoder = new streams.BlockEncoder(t, {codec: 'deflate'}); | ||
| var decoder = new streams.BlockDecoder({maxDecompressLength: Infinity}) | ||
| .on('data', function (s) { out.push(s); }) | ||
| .on('end', function () { | ||
| restore(); | ||
| assert.deepEqual(out, [payload]); | ||
| cb(); | ||
| }); |
There was a problem hiding this comment.
Fixed — added an 'error' handler that calls restore() (and fails via cb(err)), so AVRO_MAX_DECOMPRESS_LENGTH is restored on both the success and error paths.
There was a problem hiding this comment.
Already handled — restore() runs on both the 'error' and 'end' paths (test_files.js:586-588).
The fallback test only restored AVRO_MAX_DECOMPRESS_LENGTH on the 'end' event. If the decoder emitted 'error', the env var stayed modified and could cascade into later tests. Add an 'error' handler that restores it (and fails the test), so the env is restored on both success and failure. Assisted-by: GitHub Copilot:claude-opus-4.8
| if (err) { | ||
| self.emit('error', err); | ||
| // If the writable side already finished and this was the last pending | ||
| // block, end the readable side too so a consumer waiting on the queue is | ||
| // not left hanging after the error. | ||
| if (self._needPush && self._finished && !self._pending) { | ||
| self._needPush = false; | ||
| self.push(null); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The decoder's error contract is to emit 'error' (consumers listen for it), which the size-safeguard path does. Switching to destroy(err) changes the stream lifecycle (emits 'close', marks destroyed) and, for a piped source, cleaning up the upstream ReadStream on a destination error is really the caller's responsibility — stream.pipeline() does exactly that and is the recommended way to wire these up. Changing the decoder's teardown semantics late in this PR risks affecting existing consumers, so I'd prefer to keep the emit-'error' contract here; a destroy()/pipeline-based cleanup can be a focused follow-up if desired.
There was a problem hiding this comment.
Agreed this is worth doing, but calling destroy() interacts with the hand-rolled finish/push(null) lifecycle in BlockDecoder and needs careful testing, so it's out of scope for this decompression-limit PR. Tracked as a follow-up: AVRO-4307 ("BlockDecoder should destroy itself on decompression error to stop upstream I/O").
| it('caps a custom deflate codec that reuses zlib.inflateRaw', function (cb) { | ||
| // A custom codecs map that reuses the built-in raw inflater must still get | ||
| // the zlib maxOutputLength cap (not just the post-decompress check). | ||
| var zlib = require('zlib'); | ||
| var t = createType('string'); | ||
| var big = new Array(100001).join('a'); // 100000 bytes when decompressed | ||
| var encoder = new streams.BlockEncoder(t, {codec: 'deflate'}); | ||
| var decoder = new streams.BlockDecoder({ | ||
| codecs: {deflate: zlib.inflateRaw}, | ||
| maxDecompressLength: 1024 | ||
| }) | ||
| .on('data', function () {}) | ||
| .on('error', function (err) { | ||
| assert.equal(err.code, 'ERR_BUFFER_TOO_LARGE'); | ||
| cb(); | ||
| }); | ||
| encoder.pipe(decoder); | ||
| encoder.end(big); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Added — a new 'normalizes decompress limits' test exercises the finite-but-over-cap path (normalize(cap + 1) === cap, normalize(Number.MAX_VALUE) === cap) plus flooring, string parsing, and the invalid/out-of-range fallbacks. normalizeMaxDecompressLength/MAX_DECOMPRESS_LENGTH_CAP are now exported for tests.
…ests Export normalizeMaxDecompressLength and MAX_DECOMPRESS_LENGTH_CAP for tests and add unit coverage for the normalization paths, including the previously untested "finite but > cap" case (clamps to MAX_DECOMPRESS_LENGTH_CAP), flooring, string parsing, and the invalid/out-of-range fallbacks to undefined. Assisted-by: GitHub Copilot:claude-opus-4.8
What is the purpose of the change
The built-in deflate codec passes the limit to zlib (via
maxOutputLength) so the allocation itself is capped, and every codec's output is additionally checked as a safeguard. The limit can also be set per decoder via themaxDecompressLengthoption.When reading a data file, each block is decompressed according to the file's codec. A block with a very high compression ratio (or a malformed block) could expand to far more memory than its compressed size (a decompression bomb). This enforces a configurable maximum decompressed size while reading each block, mirroring the Java SDK's decompression limit (AVRO-4247). The limit defaults to 200 MiB and can be overridden with the
AVRO_MAX_DECOMPRESS_LENGTHenvironment variable.This is part of the umbrella issue AVRO-4283.
Verifying this change
This change added tests and can be verified as follows:
test/test_files.js: a deflate block exceeding the limit is rejected, a within-limit block decodes, the limit is enforced for custom codecs (including a custom codec that reuseszlib.inflateRaw), an out-of-range limit falls back to the default, andnormalizeMaxDecompressLengthis unit-tested.cd lang/js && npm testDocumentation
AVRO_MAX_DECOMPRESS_LENGTHenvironment variable is documented in code comments)