From 252f37630a08cfb74ee1a1fe3e60cf82c41d7984 Mon Sep 17 00:00:00 2001 From: Andrey Pechkurov Date: Tue, 23 Jun 2020 17:18:31 +0300 Subject: [PATCH] zlib: switch to lazy init for zlib streams PR-URL: https://github.com/nodejs/node/pull/34048 Reviewed-By: Anna Henningsen --- lib/zlib.js | 22 ++++--- src/node_zlib.cc | 59 ++++++++++++++----- test/parallel/test-zlib-reset-before-write.js | 37 ++++++++++++ 3 files changed, 92 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-zlib-reset-before-write.js diff --git a/lib/zlib.js b/lib/zlib.js index 7cc8e2e6275041..829a472e35b6a4 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -659,18 +659,13 @@ function Zlib(opts, mode) { // to come up with a good solution that doesn't break our internal API, // and with it all supported npm versions at the time of writing. this._writeState = new Uint32Array(2); - if (!handle.init(windowBits, - level, - memLevel, - strategy, - this._writeState, - processCallback, - dictionary)) { - // TODO(addaleax): Sometimes we generate better error codes in C++ land, - // e.g. ERR_BROTLI_PARAM_SET_FAILED -- it's hard to access them with - // the current bindings setup, though. - throw new ERR_ZLIB_INITIALIZATION_FAILED(); - } + handle.init(windowBits, + level, + memLevel, + strategy, + this._writeState, + processCallback, + dictionary); ZlibBase.call(this, opts, mode, handle, zlibDefaultOpts); @@ -816,6 +811,9 @@ function Brotli(opts, mode) { new binding.BrotliDecoder(mode) : new binding.BrotliEncoder(mode); this._writeState = new Uint32Array(2); + // TODO(addaleax): Sometimes we generate better error codes in C++ land, + // e.g. ERR_BROTLI_PARAM_SET_FAILED -- it's hard to access them with + // the current bindings setup, though. if (!handle.init(brotliInitParamsArray, this._writeState, processCallback)) { diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 85e87327c07797..efb11debf8f40d 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -139,8 +139,8 @@ class ZlibContext : public MemoryRetainer { CompressionError ResetStream(); // Zlib-specific: - CompressionError Init(int level, int window_bits, int mem_level, int strategy, - std::vector&& dictionary); + void Init(int level, int window_bits, int mem_level, int strategy, + std::vector&& dictionary); void SetAllocationFunctions(alloc_func alloc, free_func free, void* opaque); CompressionError SetParams(int level, int strategy); @@ -157,7 +157,10 @@ class ZlibContext : public MemoryRetainer { private: CompressionError ErrorForMessage(const char* message) const; CompressionError SetDictionary(); + bool InitZlib(); + Mutex mutex_; // Protects zlib_init_done_. + bool zlib_init_done_ = false; int err_ = 0; int flush_ = 0; int level_ = 0; @@ -615,13 +618,8 @@ class ZlibStream : public CompressionStream { AllocScope alloc_scope(wrap); wrap->context()->SetAllocationFunctions( AllocForZlib, FreeForZlib, static_cast(wrap)); - const CompressionError err = - wrap->context()->Init(level, window_bits, mem_level, strategy, - std::move(dictionary)); - if (err.IsError()) - wrap->EmitError(err); - - return args.GetReturnValue().Set(!err.IsError()); + wrap->context()->Init(level, window_bits, mem_level, strategy, + std::move(dictionary)); } static void Params(const FunctionCallbackInfo& args) { @@ -724,6 +722,15 @@ using BrotliEncoderStream = BrotliCompressionStream; using BrotliDecoderStream = BrotliCompressionStream; void ZlibContext::Close() { + { + Mutex::ScopedLock lock(mutex_); + if (!zlib_init_done_) { + dictionary_.clear(); + mode_ = NONE; + return; + } + } + CHECK_LE(mode_, UNZIP); int status = Z_OK; @@ -742,6 +749,11 @@ void ZlibContext::Close() { void ZlibContext::DoThreadPoolWork() { + bool first_init_call = InitZlib(); + if (first_init_call && err_ != Z_OK) { + return; + } + const Bytef* next_expected_header_byte = nullptr; // If the avail_out is left at 0, then it means that it ran out @@ -897,6 +909,11 @@ CompressionError ZlibContext::GetErrorInfo() const { CompressionError ZlibContext::ResetStream() { + bool first_init_call = InitZlib(); + if (first_init_call && err_ != Z_OK) { + return ErrorForMessage("Failed to init stream before reset"); + } + err_ = Z_OK; switch (mode_) { @@ -930,7 +947,7 @@ void ZlibContext::SetAllocationFunctions(alloc_func alloc, } -CompressionError ZlibContext::Init( +void ZlibContext::Init( int level, int window_bits, int mem_level, int strategy, std::vector&& dictionary) { if (!((window_bits == 0) && @@ -974,6 +991,15 @@ CompressionError ZlibContext::Init( window_bits_ *= -1; } + dictionary_ = std::move(dictionary); +} + +bool ZlibContext::InitZlib() { + Mutex::ScopedLock lock(mutex_); + if (zlib_init_done_) { + return false; + } + switch (mode_) { case DEFLATE: case GZIP: @@ -995,15 +1021,15 @@ CompressionError ZlibContext::Init( UNREACHABLE(); } - dictionary_ = std::move(dictionary); - if (err_ != Z_OK) { dictionary_.clear(); mode_ = NONE; - return ErrorForMessage("zlib error"); + return true; } - return SetDictionary(); + SetDictionary(); + zlib_init_done_ = true; + return true; } @@ -1040,6 +1066,11 @@ CompressionError ZlibContext::SetDictionary() { CompressionError ZlibContext::SetParams(int level, int strategy) { + bool first_init_call = InitZlib(); + if (first_init_call && err_ != Z_OK) { + return ErrorForMessage("Failed to init stream before set parameters"); + } + err_ = Z_OK; switch (mode_) { diff --git a/test/parallel/test-zlib-reset-before-write.js b/test/parallel/test-zlib-reset-before-write.js new file mode 100644 index 00000000000000..6b2b107d25d313 --- /dev/null +++ b/test/parallel/test-zlib-reset-before-write.js @@ -0,0 +1,37 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const zlib = require('zlib'); + +// Tests that zlib streams support .reset() and .params() +// before the first write. That is important to ensure that +// lazy init of zlib native library handles these cases. + +for (const fn of [ + (z, cb) => { + z.reset(); + cb(); + }, + (z, cb) => z.params(0, zlib.constants.Z_DEFAULT_STRATEGY, cb) +]) { + const deflate = zlib.createDeflate(); + const inflate = zlib.createInflate(); + + deflate.pipe(inflate); + + const output = []; + inflate + .on('error', (err) => { + assert.ifError(err); + }) + .on('data', (chunk) => output.push(chunk)) + .on('end', common.mustCall( + () => assert.deepStrictEqual(Buffer.concat(output).toString(), 'abc'))); + + fn(deflate, () => { + fn(inflate, () => { + deflate.write('abc'); + deflate.end(); + }); + }); +}