Skip to content

Commit 9046035

Browse files
Ic3b3rgrichardlau
Ic3b3rg
authored and
richardlau
committed
zlib: validate flush king for all streams
Signed-off-by: Ic3b3rg <s.ceccarini94@gmail.com> PR-URL: #63746 Fixes: #63701 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 98be430 commit 9046035

2 files changed

Lines changed: 72 additions & 56 deletions

File tree

‎lib/zlib.js‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -358,17 +358,10 @@ ZlibBase.prototype.flush = function(kind, callback) {
358358
kind = this._defaultFullFlushFlag;
359359
}
360360

361-
// Reject kinds outside the brotli operation range. Otherwise the fake-chunk
362-
// path forwards an unsupported flush flag to the native encoder/decoder,
363-
// which spins at 100% CPU without making progress (see #63701).
364-
if (this._flushBoundIdx === FLUSH_BOUND_IDX_BROTLI) {
365-
const min = FLUSH_BOUND[FLUSH_BOUND_IDX_BROTLI][0];
366-
const max = FLUSH_BOUND[FLUSH_BOUND_IDX_BROTLI][1];
367-
if (typeof kind !== 'number' || NumberIsNaN(kind) ||
368-
kind < min || kind > max) {
369-
throw new ERR_OUT_OF_RANGE('kind', `>= ${min} and <= ${max}`, kind);
370-
}
371-
}
361+
kind = checkRangesOrGetDefault(
362+
kind, 'kind',
363+
FLUSH_BOUND[this._flushBoundIdx][0], FLUSH_BOUND[this._flushBoundIdx][1],
364+
this._defaultFullFlushFlag);
372365

373366
if (this.writableFinished) {
374367
if (callback)

‎test/parallel/test-zlib-brotli-flush-invalid-kind.js‎

Lines changed: 68 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
'use strict';
2-
// Regression test for https://github.com/nodejs/node/issues/63701
3-
// BrotliCompress/BrotliDecompress.flush(kind) used to spin at 100% CPU when
4-
// kind was outside the brotli operation range (0..3) — e.g. Z_FINISH (4) or
5-
// Z_BLOCK (5). It must now throw ERR_OUT_OF_RANGE before reaching the native
6-
// layer.
2+
// Regression test for https://github.com/nodejs/node/issues/63701.
3+
// Invalid Brotli flush kinds used to spin in native code. flush(kind) should
4+
// reject invalid kinds before writing the fake flush chunk.
75

86
require('../common');
97
const assert = require('assert');
@@ -14,56 +12,81 @@ const {
1412
BROTLI_OPERATION_FLUSH,
1513
BROTLI_OPERATION_FINISH,
1614
BROTLI_OPERATION_EMIT_METADATA,
15+
Z_NO_FLUSH,
1716
Z_BLOCK,
1817
Z_FINISH,
18+
ZSTD_e_continue,
19+
ZSTD_e_flush,
20+
ZSTD_e_end,
1921
} = zlib.constants;
2022

21-
// Brotli operations 0..3 are valid and must not throw synchronously from
22-
// flush(). Some operations (e.g. FINISH on a decoder with no input) emit a
23-
// Z_BUF_ERROR asynchronously — that's expected and unrelated to the input
24-
// validation under test, so we attach a noop error handler.
25-
for (const validKind of [
26-
BROTLI_OPERATION_PROCESS,
27-
BROTLI_OPERATION_FLUSH,
28-
BROTLI_OPERATION_FINISH,
29-
BROTLI_OPERATION_EMIT_METADATA,
30-
]) {
31-
const c = zlib.createBrotliCompress();
32-
c.on('error', () => {});
33-
c.flush(validKind);
23+
const noop = () => {};
3424

35-
const d = zlib.createBrotliDecompress();
36-
d.on('error', () => {});
37-
d.flush(validKind);
38-
}
25+
const flushKindTestCases = [
26+
{
27+
factories: [zlib.createGzip],
28+
validKinds: [Z_NO_FLUSH, Z_FINISH, Z_BLOCK],
29+
invalidKinds: [-1, 6, 100],
30+
},
31+
{
32+
factories: [zlib.createBrotliCompress, zlib.createBrotliDecompress],
33+
validKinds: [
34+
BROTLI_OPERATION_PROCESS,
35+
BROTLI_OPERATION_FLUSH,
36+
BROTLI_OPERATION_FINISH,
37+
BROTLI_OPERATION_EMIT_METADATA,
38+
],
39+
invalidKinds: [-1, Z_FINISH, Z_BLOCK, 6, 100],
40+
},
41+
{
42+
factories: [zlib.createZstdCompress, zlib.createZstdDecompress],
43+
validKinds: [ZSTD_e_continue, ZSTD_e_flush, ZSTD_e_end],
44+
invalidKinds: [-1, 3, Z_FINISH, Z_BLOCK, 100],
45+
},
46+
];
3947

40-
// Values outside [0, 3] must throw ERR_OUT_OF_RANGE for both compress and
41-
// decompress streams. Z_FINISH (4) and Z_BLOCK (5) previously hung at 100% CPU.
42-
const outOfRange = [-1, Z_FINISH, Z_BLOCK, 6, 7, 100];
48+
for (const { factories, validKinds } of flushKindTestCases) {
49+
for (const factory of factories) {
50+
for (const kind of validKinds) {
51+
const stream = factory();
52+
stream.on('error', noop);
53+
stream.flush(kind);
54+
}
55+
}
56+
}
4357

44-
for (const factory of [zlib.createBrotliCompress, zlib.createBrotliDecompress]) {
45-
for (const kind of outOfRange) {
46-
assert.throws(
47-
() => factory().flush(kind),
48-
{ code: 'ERR_OUT_OF_RANGE', name: 'RangeError' },
49-
);
58+
for (const { factories, invalidKinds } of flushKindTestCases) {
59+
for (const factory of factories) {
60+
for (const kind of invalidKinds) {
61+
assert.throws(
62+
() => factory().flush(kind),
63+
{ code: 'ERR_OUT_OF_RANGE', name: 'RangeError' },
64+
);
65+
}
5066
}
5167
}
5268

53-
// Non-number kinds must also throw, instead of silently triggering the
54-
// fake-chunk path with an undefined buffer.
55-
for (const factory of [zlib.createBrotliCompress, zlib.createBrotliDecompress]) {
56-
for (const kind of ['foobar', null, {}, NaN]) {
57-
assert.throws(
58-
() => factory().flush(kind),
59-
{ code: 'ERR_OUT_OF_RANGE' },
60-
);
69+
for (const { factories } of flushKindTestCases) {
70+
for (const factory of factories) {
71+
for (const kind of ['foobar', null, {}]) {
72+
assert.throws(
73+
() => factory().flush(kind),
74+
{ code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError' },
75+
);
76+
}
6177
}
6278
}
6379

64-
// flush() with no arguments (or with only a callback) must still work —
65-
// it uses the stream's _defaultFullFlushFlag, which is a valid brotli op.
66-
zlib.createBrotliCompress().flush();
67-
zlib.createBrotliCompress().flush(() => {});
68-
zlib.createBrotliDecompress().flush();
69-
zlib.createBrotliDecompress().flush(() => {});
80+
for (const { factories } of flushKindTestCases) {
81+
for (const factory of factories) {
82+
for (const kind of [undefined, NaN]) {
83+
const stream = factory();
84+
stream.on('error', noop);
85+
stream.flush(kind);
86+
}
87+
88+
const stream = factory();
89+
stream.on('error', noop);
90+
stream.flush(noop);
91+
}
92+
}

0 commit comments

Comments
 (0)