Improve parser error handling

- Wrap parser errors with VError
- Catch initial potential sequence errors
- Improve parser test coverage
- Emit parser errors instead of implicitly logging

Fix mcavage/node-ldapjs#221

lib/client/client.js

@@ -4,11 +4,12 @@ var EventEmitter = require('events').EventEmitter;
 var net = require('net');
 var tls = require('tls');
 var util = require('util');
+
 var once = require('once');
 var backoff = require('backoff');
 var vasync = require('vasync');
-
 var assert = require('assert-plus');
+var VError = require('verror').VError;
 
 var Attribute = require('../attribute');
 var Change = require('../change');
@@ -986,8 +987,8 @@ Client.prototype.connect = function connect() {
     });
 
     socket.ldap.parser.on('error', function onParseError(err) {
-      log.trace({err: err}, 'parser error event');
+      self.emit('error', new VError(err, 'Parser error for %s',
-      self.emit('error', err);
+            socket.ldap.id));
       self.connected = false;
       socket.end();
     });

lib/messages/parser.js

@@ -1,10 +1,11 @@
 // Copyright 2011 Mark Cavage, Inc.  All rights reserved.
 
-var assert = require('assert');
 var EventEmitter = require('events').EventEmitter;
 var util = require('util');
 
+var assert = require('assert-plus');
 var asn1 = require('asn1');
+var VError = require('verror').VError;
 
 var AbandonRequest = require('./abandon_request');
 var AddRequest = require('./add_request');
@@ -76,12 +77,19 @@ Parser.prototype.write = function (data) {
   self.buffer = (self.buffer ? Buffer.concat([self.buffer, data]) : data);
 
   var ber = new BerReader(self.buffer);
-  if (!ber.readSequence())
-    return false;
 
-  if (ber.remain < ber.length) { // ENOTENOUGH
+  var foundSeq = false;
+  try {
+    foundSeq = ber.readSequence();
+  } catch (e) {
+    this.emit('error', e);
+  }
+
+  if (!foundSeq || ber.remain < ber.length) {
+    // ENOTENOUGH
     return false;
-  } else if (ber.remain > ber.length) { // ETOOMUCH
+  } else if (ber.remain > ber.length) {
+    // ETOOMUCH
     // This is sort of ugly, but allows us to make miminal copies
     nextMessage = self.buffer.slice(ber.offset + ber.length);
     ber._size = ber.offset + ber.length;
@@ -92,18 +100,21 @@ Parser.prototype.write = function (data) {
   // pointing at the next sequence of data (if it exists)
   self.buffer = null;
 
-  var message = this.getMessage(ber);
+  var message;
-  if (!message)
-    return end();
-
   try {
+    // Bail here if peer isn't speaking protocol at all
+    message = this.getMessage(ber);
+
+    if (!message) {
+      return end();
+    }
     message.parse(ber);
   } catch (e) {
     this.emit('error', e, message);
     return false;
   }
-  this.emit('message', message);
 
+  this.emit('message', message);
   return end();
 };
 
@@ -113,14 +124,8 @@ Parser.prototype.getMessage = function (ber) {
 
   var self = this;
 
-  try {
   var messageID = ber.readInt();
   var type = ber.readSequence();
-  } catch (e) {
-    // Handle servers that aren't speaking the language at all
-    this.emit('error', e);
-    return false;
-  }
 
   var Message;
   switch (type) {
@@ -207,8 +212,7 @@ Parser.prototype.getMessage = function (ber) {
 
   default:
     this.emit('error',
-              new Error('protocolOp 0x' +
+              new Error('Op 0x' + (type ? type.toString(16) : '??') +
-                        (type ? type.toString(16) : '??') +
                         ' not supported'),
               new LDAPResult({
                 messageID: messageID,

lib/server.js

@@ -7,6 +7,7 @@ var tls = require('tls');
 var util = require('util');
 
 var asn1 = require('asn1');
+var VError = require('verror').VError;
 
 var dn = require('./dn');
 var dtrace = require('./dtrace');
@@ -284,7 +285,8 @@ function Server(options) {
     if (c.type === 'unix') {
       c.remoteAddress = self.server.path;
       c.remotePort = c.fd;
-    } else if (c.socket) { // TLS
+    } else if (c.socket) {
+      // TLS
       c.remoteAddress = c.socket.remoteAddress;
       c.remotePort = c.socket.remotePort;
     }
@@ -406,9 +408,7 @@ function Server(options) {
     });
 
     c.parser.on('error', function (err, message) {
-      log.error('Exception happened parsing for %s: %s',
+      self.emit('error', new VError(err, 'Parser error for %s', c.ldap.id));
-                c.ldap.id, err.stack);
-
 
       if (!message)
         return c.destroy();
@@ -426,16 +426,7 @@ function Server(options) {
       if (log.trace())
         log.trace('data on %s: %s', c.ldap.id, util.inspect(data));
 
-      try {
       c.parser.write(data);
-      } catch (e) {
-        log.warn('Unable to parse message [c.on(\'data\')]: %s', e.stack);
-        c.end(new LDAPResult({
-          status: 0x02,
-          errorMessage: e.toString(),
-          connection: c
-        }).toBer());
-      }
     });
 
   } // end newConnection

package.json

@@ -33,7 +33,8 @@
     "dashdash": "1.6.0",
     "backoff": "2.4.0",
     "once": "1.3.0",
-    "vasync": "1.5.0"
+    "vasync": "1.5.0",
+    "verror": "~1.4.0"
   },
   "optionalDependencies": {
     "dtrace-provider": "0.2.8"

test/messages/parser.test.js

@@ -0,0 +1,64 @@
+// Copyright 2014 Joyent, Inc.  All rights reserved.
+
+var test = require('tape').test;
+var bunyan = require('bunyan');
+
+///--- Globals
+
+var lib;
+var Parser;
+var LOG = bunyan.createLogger({name: 'ldapjs-test'});
+
+///--- Tests
+test('load library', function (t) {
+  lib = require('../../lib/');
+  Parser = lib.Parser;
+
+  t.ok(Parser);
+  t.end();
+});
+
+test('wrong protocol error', function (t) {
+  var p = new Parser({log: LOG});
+
+  p.once('error', function (err) {
+    t.ok(err);
+    t.end();
+  });
+
+  // Send some bogus data to incur an error
+  p.write(new Buffer([16, 1, 4]));
+});
+
+test('bad protocol op', function (t) {
+  var p = new Parser({log: LOG});
+  var message = new lib.LDAPMessage({
+    protocolOp: 254 // bogus (at least today)
+  });
+  p.once('error', function (err) {
+    t.ok(err);
+    t.ok(/not supported$/.test(err.message));
+    t.end();
+  });
+  p.write(message.toBer());
+});
+
+test('bad message structure', function (t) {
+  var p = new Parser({log: LOG});
+
+  // message with bogus structure
+  var message = new lib.LDAPMessage({
+    protocolOp: lib.LDAP_REQ_EXTENSION
+  });
+  message._toBer = function (writer) {
+    writer.writeBuffer(new Buffer([16, 1, 4]), 80);
+    return writer;
+  };
+
+  p.once('error', function (err) {
+    t.ok(err);
+    t.end();
+  });
+
+  p.write(message.toBer());
+});