Fix #3052 Handle errors properly in zlib

nodejs · Apr 2, 2012 · 01d46f3 · 01d46f3
1 parent cc15299
commit 01d46f3
Show file tree

Hide file tree

Showing 3 changed files with 157 additions and 32 deletions.
diff --git a/lib/zlib.js b/lib/zlib.js
@@ -50,6 +50,22 @@ Object.keys(binding).forEach(function(k) {
   if (k.match(/^Z/)) exports[k] = binding[k];
 });
 
+// translation table for return codes.
+exports.codes = {
+  Z_OK: binding.Z_OK,
+  Z_STREAM_END: binding.Z_STREAM_END,
+  Z_NEED_DICT: binding.Z_NEED_DICT,
+  Z_ERRNO: binding.Z_ERRNO,
+  Z_STREAM_ERROR: binding.Z_STREAM_ERROR,
+  Z_DATA_ERROR: binding.Z_DATA_ERROR,
+  Z_MEM_ERROR: binding.Z_MEM_ERROR,
+  Z_BUF_ERROR: binding.Z_BUF_ERROR,
+  Z_VERSION_ERROR: binding.Z_VERSION_ERROR
+};
+
+Object.keys(exports.codes).forEach(function(k) {
+  exports.codes[exports.codes[k]] = k;
+});
 
 exports.Deflate = Deflate;
 exports.Inflate = Inflate;
@@ -270,6 +286,22 @@ function Zlib(opts, mode) {
   }
 
   this._binding = new binding.Zlib(mode);
+
+  var self = this;
+  this._binding.onerror = function(message, errno) {
+    // there is no way to cleanly recover.
+    // continuing only obscures problems.
+    self._binding = null;
+    self._hadError = true;
+    self._queue.length = 0;
+    self._processing = false;
+
+    var error = new Error(message);
+    error.errno = errno;
+    error.code = exports.codes[errno];
+    self.emit('error', error);
+  };
+
   this._binding.init(opts.windowBits || exports.Z_DEFAULT_WINDOWBITS,
                      opts.level || exports.Z_DEFAULT_COMPRESSION,
                      opts.memLevel || exports.Z_DEFAULT_MEMLEVEL,
@@ -285,6 +317,8 @@ function Zlib(opts, mode) {
 util.inherits(Zlib, stream.Stream);
 
 Zlib.prototype.write = function write(chunk, cb) {
+  if (this._hadError) return true;
+
   if (this._ended) {
     return this.emit('error', new Error('Cannot write after end'));
   }
@@ -323,6 +357,8 @@ Zlib.prototype.flush = function flush(cb) {
 };
 
 Zlib.prototype.end = function end(chunk, cb) {
+  if (this._hadError) return true;
+
   var self = this;
   this._ending = true;
   var ret = this.write(chunk, function() {
@@ -334,6 +370,8 @@ Zlib.prototype.end = function end(chunk, cb) {
 };
 
 Zlib.prototype._process = function() {
+  if (this._hadError) return;
+
   if (this._processing || this._paused) return;
 
   if (this._queue.length === 0) {
@@ -371,6 +409,8 @@ Zlib.prototype._process = function() {
   this._processing = req;
 
   function callback(availInAfter, availOutAfter, buffer) {
+    if (self._hadError) return;
+
     var have = availOutBefore - availOutAfter;
 
     assert(have >= 0, 'have should not go down');

diff --git a/src/node_zlib.cc b/src/node_zlib.cc
@@ -36,6 +36,7 @@ using namespace v8;
 
 
 static Persistent<String> callback_sym;
+static Persistent<String> onerror_sym;
 
 enum node_zlib_mode {
   DEFLATE = 1,
@@ -142,37 +143,41 @@ class ZCtx : public ObjectWrap {
     // If the avail_out is left at 0, then it means that it ran out
     // of room.  If there was avail_out left over, then it means
     // that all of the input was consumed.
-    int err = Z_STREAM_ERROR;
     switch (ctx->mode_) {
       case DEFLATE:
       case GZIP:
       case DEFLATERAW:
-        err = deflate(&ctx->strm_, ctx->flush_);
+        ctx->err_ = deflate(&ctx->strm_, ctx->flush_);
         break;
       case UNZIP:
       case INFLATE:
       case GUNZIP:
       case INFLATERAW:
-        err = inflate(&ctx->strm_, ctx->flush_);
+        ctx->err_ = inflate(&ctx->strm_, ctx->flush_);
 
         // If data was encoded with dictionary
-        if (err == Z_NEED_DICT) {
+        if (ctx->err_ == Z_NEED_DICT) {
           assert(ctx->dictionary_ != NULL && "Stream has no dictionary");
-
-          // Load it
-          err = inflateSetDictionary(&ctx->strm_,
-                                     ctx->dictionary_,
-                                     ctx->dictionary_len_);
-          assert(err == Z_OK && "Failed to set dictionary");
-
-          // And try to decode again
-          err = inflate(&ctx->strm_, ctx->flush_);
+          if (ctx->dictionary_ != NULL) {
+
+            // Load it
+            ctx->err_ = inflateSetDictionary(&ctx->strm_,
+                                             ctx->dictionary_,
+                                             ctx->dictionary_len_);
+            assert(ctx->err_ == Z_OK && "Failed to set dictionary");
+            if (ctx->err_ == Z_OK) {
+
+              // And try to decode again
+              ctx->err_ = inflate(&ctx->strm_, ctx->flush_);
+            }
+          }
         }
         break;
       default:
         assert(0 && "wtf?");
     }
-    assert(err != Z_STREAM_ERROR);
+
+    // pass any errors back to the main thread to deal with.
 
     // now After will emit the output, and
     // either schedule another call to Process,
@@ -184,6 +189,19 @@ class ZCtx : public ObjectWrap {
     HandleScope scope;
     ZCtx *ctx = container_of(work_req, ZCtx, work_req_);
 
+    // Acceptable error states depend on the type of zlib stream.
+    switch (ctx->err_) {
+      case Z_OK:
+      case Z_STREAM_END:
+      case Z_BUF_ERROR:
+        // normal statuses, not fatal
+        break;
+      default:
+        // something else.
+        ZCtx::Error(ctx, "Zlib error");
+        return;
+    }
+
     Local<Integer> avail_out = Integer::New(ctx->strm_.avail_out);
     Local<Integer> avail_in = Integer::New(ctx->strm_.avail_in);
 
@@ -198,6 +216,25 @@ class ZCtx : public ObjectWrap {
     ctx->Unref();
   }
 
+  static void Error(ZCtx *ctx, const char *msg_) {
+    const char *msg;
+    if (ctx->strm_.msg != NULL) {
+      msg = ctx->strm_.msg;
+    } else {
+      msg = msg_;
+    }
+
+    assert(ctx->handle_->Get(onerror_sym)->IsFunction() &&
+           "Invalid error handler");
+    HandleScope scope;
+    Local<Value> args[2] = { String::New(msg),
+                             Local<Value>::New(Number::New(ctx->err_)) };
+    MakeCallback(ctx->handle_, "onerror", ARRAY_SIZE(args), args);
+
+    // no hope of rescue.
+    ctx->Unref();
+  }
+
   static Handle<Value> New(const Arguments& args) {
     HandleScope scope;
     if (args.Length() < 1 || !args[0]->IsInt32()) {
@@ -279,6 +316,8 @@ class ZCtx : public ObjectWrap {
 
     ctx->flush_ = Z_NO_FLUSH;
 
+    ctx->err_ = Z_OK;
+
     if (ctx->mode_ == GZIP || ctx->mode_ == GUNZIP) {
       ctx->windowBits_ += 16;
     }
@@ -291,29 +330,31 @@ class ZCtx : public ObjectWrap {
       ctx->windowBits_ *= -1;
     }
 
-    int err;
     switch (ctx->mode_) {
       case DEFLATE:
       case GZIP:
       case DEFLATERAW:
-        err = deflateInit2(&ctx->strm_,
-                           ctx->level_,
-                           Z_DEFLATED,
-                           ctx->windowBits_,
-                           ctx->memLevel_,
-                           ctx->strategy_);
+        ctx->err_ = deflateInit2(&ctx->strm_,
+                                 ctx->level_,
+                                 Z_DEFLATED,
+                                 ctx->windowBits_,
+                                 ctx->memLevel_,
+                                 ctx->strategy_);
         break;
       case INFLATE:
       case GUNZIP:
       case INFLATERAW:
       case UNZIP:
-        err = inflateInit2(&ctx->strm_, ctx->windowBits_);
+        ctx->err_ = inflateInit2(&ctx->strm_, ctx->windowBits_);
         break;
       default:
         assert(0 && "wtf?");
     }
 
-    assert(err == Z_OK);
+    if (ctx->err_ != Z_OK) {
+      ZCtx::Error(ctx, "Init error");
+    }
+
 
     ctx->dictionary_ = reinterpret_cast<Bytef *>(dictionary);
     ctx->dictionary_len_ = dictionary_len;
@@ -325,39 +366,43 @@ class ZCtx : public ObjectWrap {
   static void SetDictionary(ZCtx* ctx) {
     if (ctx->dictionary_ == NULL) return;
 
-    int err = Z_OK;
+    ctx->err_ = Z_OK;
 
     switch (ctx->mode_) {
       case DEFLATE:
       case DEFLATERAW:
-        err = deflateSetDictionary(&ctx->strm_,
-                                   ctx->dictionary_,
-                                   ctx->dictionary_len_);
+        ctx->err_ = deflateSetDictionary(&ctx->strm_,
+                                         ctx->dictionary_,
+                                         ctx->dictionary_len_);
         break;
       default:
         break;
     }
 
-    assert(err == Z_OK && "Failed to set dictionary");
+    if (ctx->err_ != Z_OK) {
+      ZCtx::Error(ctx, "Failed to set dictionary");
+    }
   }
 
   static void Reset(ZCtx* ctx) {
-    int err = Z_OK;
+    ctx->err_ = Z_OK;
 
     switch (ctx->mode_) {
       case DEFLATE:
       case DEFLATERAW:
-        err = deflateReset(&ctx->strm_);
+        ctx->err_ = deflateReset(&ctx->strm_);
         break;
       case INFLATE:
       case INFLATERAW:
-        err = inflateReset(&ctx->strm_);
+        ctx->err_ = inflateReset(&ctx->strm_);
         break;
       default:
         break;
     }
 
-    assert(err == Z_OK && "Failed to reset stream");
+    if (ctx->err_ != Z_OK) {
+      ZCtx::Error(ctx, "Failed to reset stream");
+    }
   }
 
  private:
@@ -370,6 +415,8 @@ class ZCtx : public ObjectWrap {
   int memLevel_;
   int strategy_;
 
+  int err_;
+
   Bytef* dictionary_;
   size_t dictionary_len_;
 
@@ -399,13 +446,16 @@ void InitZlib(Handle<Object> target) {
   target->Set(String::NewSymbol("Zlib"), z->GetFunction());
 
   callback_sym = NODE_PSYMBOL("callback");
+  onerror_sym = NODE_PSYMBOL("onerror");
 
   NODE_DEFINE_CONSTANT(target, Z_NO_FLUSH);
   NODE_DEFINE_CONSTANT(target, Z_PARTIAL_FLUSH);
   NODE_DEFINE_CONSTANT(target, Z_SYNC_FLUSH);
   NODE_DEFINE_CONSTANT(target, Z_FULL_FLUSH);
   NODE_DEFINE_CONSTANT(target, Z_FINISH);
   NODE_DEFINE_CONSTANT(target, Z_BLOCK);
+
+  // return/error codes
   NODE_DEFINE_CONSTANT(target, Z_OK);
   NODE_DEFINE_CONSTANT(target, Z_STREAM_END);
   NODE_DEFINE_CONSTANT(target, Z_NEED_DICT);
@@ -415,6 +465,7 @@ void InitZlib(Handle<Object> target) {
   NODE_DEFINE_CONSTANT(target, Z_MEM_ERROR);
   NODE_DEFINE_CONSTANT(target, Z_BUF_ERROR);
   NODE_DEFINE_CONSTANT(target, Z_VERSION_ERROR);
+
   NODE_DEFINE_CONSTANT(target, Z_NO_COMPRESSION);
   NODE_DEFINE_CONSTANT(target, Z_BEST_SPEED);
   NODE_DEFINE_CONSTANT(target, Z_BEST_COMPRESSION);

diff --git a/test/simple/test-zlib-invalid-input.js b/test/simple/test-zlib-invalid-input.js
@@ -27,6 +27,7 @@ var common = require('../common.js'),
 
 var nonStringInputs = [1, true, {a: 1}, ['a']];
 
+console.error('Doing the non-strings');
 nonStringInputs.forEach(function(input) {
   // zlib.gunzip should not throw an error when called with bad input.
   assert.doesNotThrow(function() {
@@ -36,3 +37,36 @@ nonStringInputs.forEach(function(input) {
     });
   });
 });
+
+console.error('Doing the unzips');
+// zlib.Unzip classes need to get valid data, or else they'll throw.
+var unzips = [ zlib.Unzip(),
+               zlib.Gunzip(),
+               zlib.Inflate(),
+               zlib.InflateRaw() ];
+var hadError = [];
+unzips.forEach(function (uz, i) {
+  console.error('Error for '+uz.constructor.name);
+  uz.on('error', function(er) {
+    console.error('Error event', er);
+    hadError[i] = true;
+
+    // to be friendly to the Stream API, zlib objects just return true and
+    // ignore data on the floor after an error.  It's up to the user to
+    // catch the 'error' event and do something intelligent.  They do not
+    // emit any more data, however.
+    assert.equal(uz.write('also invalid'), true);
+    assert.equal(uz.end(), true);
+  });
+
+  uz.on('end', function(er) {
+    throw new Error('end event should not be emitted '+uz.constructor.name);
+  });
+
+  // this will trigger error event
+  uz.write('this is not valid compressed data.');
+});
+
+process.on('exit', function() {
+  assert.deepEqual(hadError, [true, true, true, true], 'expect 4 errors');
+});