From 7d7853369c4236082bb59333392b638007a54276 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Thu, 18 May 2017 12:49:08 +0300 Subject: [PATCH 1/6] zlib: fix node crashing on invalid options The main reason behind this commit is fixing the Node process crashing when zlib rejects the given options. Besides that issue, which got reported and which is linked to this commit, it turned out that Node also used to crash when a non-numeric value was passed as the `windowBits` or the `memLevel` option. This was fixed somewhat inadvertently; initially it was just a stylistic change to avoid lines spanning longer than 80 characters that was written in a manner consistent with surrounding code. Fixes: https://github.com/nodejs/node/issues/13082 --- lib/zlib.js | 37 ++++++++++++++++++++------ src/node_zlib.cc | 14 +++++++--- test/parallel/test-zlib-failed-init.js | 15 +++++++++++ 3 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 test/parallel/test-zlib-failed-init.js diff --git a/lib/zlib.js b/lib/zlib.js index 00be56dffc44fb..8804034017eb12 100644 --- a/lib/zlib.js +++ b/lib/zlib.js @@ -146,7 +146,14 @@ function zlibOnError(message, errno) { var error = new Error(message); error.errno = errno; error.code = codes[errno]; - this.emit('error', error); + + if (this._initSuccess) { + this.emit('error', error); + } else { + process.nextTick(() => { + this.emit('error', error); + }); + } } function flushCallback(level, strategy, callback) { @@ -222,6 +229,7 @@ class Zlib extends Transform { this._handle = new binding.Zlib(mode); this._handle.onerror = zlibOnError.bind(this); this._hadError = false; + this._initSuccess = false; var level = constants.Z_DEFAULT_COMPRESSION; if (typeof opts.level === 'number') level = opts.level; @@ -229,13 +237,24 @@ class Zlib extends Transform { var strategy = constants.Z_DEFAULT_STRATEGY; if (typeof opts.strategy === 'number') strategy = opts.strategy; - this._handle.init(opts.windowBits || constants.Z_DEFAULT_WINDOWBITS, - level, - opts.memLevel || constants.Z_DEFAULT_MEMLEVEL, - strategy, - opts.dictionary); + var windowBits = constants.Z_DEFAULT_WINDOWBITS; + if (typeof opts.windowBits === 'number') windowBits = opts.windowBits; + + var memLevel = constants.Z_DEFAULT_MEMLEVEL; + if (typeof opts.memLevel === 'number') memLevel = opts.memLevel; + + this._initSuccess = this._handle.init(windowBits, + level, + memLevel, + strategy, + opts.dictionary); + + if (this._initSuccess) { + this._buffer = Buffer.allocUnsafe(this._chunkSize); + } else { + this._buffer = null; + } - this._buffer = Buffer.allocUnsafe(this._chunkSize); this._offset = 0; this._level = level; this._strategy = strategy; @@ -467,7 +486,9 @@ function _close(engine, callback) { if (!engine._handle) return; - engine._handle.close(); + if (engine._initSuccess) + engine._handle.close(); + engine._handle = null; } diff --git a/src/node_zlib.cc b/src/node_zlib.cc index e4adda52026c87..d4e21aea17b436 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -40,6 +40,7 @@ namespace node { using v8::Array; +using v8::Boolean; using v8::Context; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -480,9 +481,11 @@ class ZCtx : public AsyncWrap { memcpy(dictionary, Buffer::Data(dictionary_), dictionary_len); } - Init(ctx, level, windowBits, memLevel, strategy, - dictionary, dictionary_len); + bool result = Init(ctx, level, windowBits, memLevel, strategy, + dictionary, dictionary_len); SetDictionary(ctx); + + args.GetReturnValue().Set(Boolean::New(args.GetIsolate(), result)); } static void Params(const FunctionCallbackInfo& args) { @@ -499,7 +502,7 @@ class ZCtx : public AsyncWrap { SetDictionary(ctx); } - static void Init(ZCtx *ctx, int level, int windowBits, int memLevel, + static bool Init(ZCtx *ctx, int level, int windowBits, int memLevel, int strategy, char* dictionary, size_t dictionary_len) { ctx->level_ = level; ctx->windowBits_ = windowBits; @@ -553,6 +556,9 @@ class ZCtx : public AsyncWrap { if (ctx->err_ != Z_OK) { ZCtx::Error(ctx, "Init error"); + if (dictionary != nullptr) + delete[] dictionary; + return false; } @@ -561,6 +567,8 @@ class ZCtx : public AsyncWrap { ctx->write_in_progress_ = false; ctx->init_done_ = true; + + return true; } static void SetDictionary(ZCtx* ctx) { diff --git a/test/parallel/test-zlib-failed-init.js b/test/parallel/test-zlib-failed-init.js new file mode 100644 index 00000000000000..b250ebffcf7bd4 --- /dev/null +++ b/test/parallel/test-zlib-failed-init.js @@ -0,0 +1,15 @@ +'use strict'; + +const common = require('../common'); + +const assert = require('assert'); +const zlib = require('zlib'); + +// For raw deflate or gzip encoding, a request for a 256-byte window is +// rejected as invalid, since only zlib headers provide means of transmitting +// the window size to the decompressor. +// (http://zlib.net/manual.html#Advanced) +const deflate = zlib.createDeflateRaw({ windowBits: 8 }); +deflate.on('error', common.mustCall((error) => { + assert.ok(/Init error/.test(error)); +})); From c5440af50c479b415aefeae798dfa0f5f570aa7a Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Thu, 18 May 2017 16:00:36 +0300 Subject: [PATCH 2/6] squash! follow an alternative approach Throw an Error synchronously instead of fiddling with 'error' events. --- doc/api/zlib.md | 4 ++++ lib/zlib.js | 31 +++++++------------------- src/node_zlib.cc | 25 +++++++++------------ test/parallel/test-zlib-failed-init.js | 14 +++++------- 4 files changed, 28 insertions(+), 46 deletions(-) diff --git a/doc/api/zlib.md b/doc/api/zlib.md index abfb6e2038b6d3..ee112ba61c6509 100644 --- a/doc/api/zlib.md +++ b/doc/api/zlib.md @@ -437,6 +437,10 @@ added: v0.5.8 Returns a new [DeflateRaw][] object with an [options][]. +**Note:** zlib library rejects requests for 256-byte windows (i.e., +`{ windowBits: 8 }` in `options`). An `Error` will be thrown when creating +a [DeflateRaw][] object with this specific value of the `windowBits` option. + ## zlib.createGunzip([options])