GH-26 rewrite network parser

lib/client.js

@@ -180,10 +180,10 @@ Client.prototype.connect = function(callback) {
         if (err)
           c.end();
 
-        self.emit('connect')
+        self.emit('connect');
       });
 
-    self.emit('connect')
+    self.emit('connect');
   });
 };
 
@@ -216,9 +216,9 @@ Client.prototype.bind = function(name, credentials, controls, callback, conn) {
   var self = this;
 
   var req = new BindRequest({
-    name: name,
+    name: name || '',
     authentication: 'Simple',
-    credentials: credentials,
+    credentials: credentials || '',
     controls: controls
   });
 
@@ -830,8 +830,9 @@ Client.prototype._newConnection = function() {
 
   c.on('connect', function() {
     if (log.isTraceEnabled())
-      log.trace('%s connect event', c.ldap.id);    c.ldap.connected = true;
+      log.trace('%s connect event', c.ldap.id);
 
+    c.ldap.connected = true;
     c.ldap.id += ':' + c.fd;
     self.emit('connect', c.ldap.id);
   });
@@ -853,7 +854,7 @@ Client.prototype._newConnection = function() {
         err = new ConnectionError(c.ldap.id + ' closed');
       } else {
         err = new UnbindResponse({
-          messageID: msgid,
+          messageID: msgid
         });
         err.status = 'unbind';
       }
@@ -910,5 +911,15 @@ Client.prototype._newConnection = function() {
     return callback(message);
   });
 
+  c.parser.on('error', function(err) {
+    if (log.isTraceEnabled())
+      log.trace('%s error event=%s', c.ldap.id, err ? err.stack : '?');
+
+    if (self.listeners('error').length)
+      self.emit('error', err);
+
+    c.end();
+  });
+
   return c;
 };

lib/messages/bind_request.js

@@ -68,9 +68,9 @@ BindRequest.prototype._toBer = function(ber) {
   assert.ok(ber);
 
   ber.writeInt(this.version);
-  ber.writeString(this.name.toString());
+  ber.writeString((this.name || '').toString());
   // TODO add support for SASL et al
-  ber.writeString(this.credentials, Ber.Context);
+  ber.writeString((this.credentials || ''), Ber.Context);
 
   return ber;
 };

lib/messages/message.js

@@ -62,17 +62,14 @@ LDAPMessage.prototype.toString = function() {
 };
 
 
-LDAPMessage.prototype.parse = function(data, length) {
-  if (!data || !Buffer.isBuffer(data))
-    throw new TypeError('data (buffer) required');
+LDAPMessage.prototype.parse = function(ber) {
+  assert.ok(ber);
 
   if (this.log.isTraceEnabled())
-    this.log.trace('parse: data=%s, len=%d', util.inspect(data), length);
-
-  var ber = new BerReader(data);
+    this.log.trace('parse: data=%s', util.inspect(ber.buffer));
 
   // Delegate off to the specific type to parse
-  this._parse(ber, length);
+  this._parse(ber, ber.remain);
 
   // Look for controls
   if (ber.peek() === 0xa0) {

lib/messages/parser.js

@@ -55,9 +55,7 @@ function Parser(options) {
 
   EventEmitter.call(this);
 
-  this._reset();
-
-  var self = this;
+  this.buffer = null;
   this.log4js = options.log4js;
   this.log = this.log4js.getLogger('Parser');
 }
@@ -69,83 +67,61 @@ Parser.prototype.write = function(data) {
   if (!data || !Buffer.isBuffer(data))
     throw new TypeError('data (buffer) required');
 
+  var log = this.log;
+  var nextMessage = null;
   var self = this;
 
-  if (this._buffer)
-    data = this._buffer.concat(data);
+  function end() {
+    if (nextMessage)
+      return self.write(nextMessage);
 
-  if (this.log.isTraceEnabled())
-    this.log.trace('Processing buffer (concat\'d): ' + util.inspect(data));
+    return true;
+  }
 
-  // If there's more than one message in this buffer
-  var extra;
+  self.buffer = (self.buffer ? self.buffer.concat(data) : data);
+
+  var ber = new BerReader(self.buffer);
+  if (!ber.readSequence())
+    return false;
+
+  if (ber.remain < ber.length) { // ENOTENOUGH
+    return false;
+  } 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;
+    assert.equal(ber.remain, ber.length);
+  }
+
+  // If we're here, ber holds the message, and nextMessage is temporarily
+  // pointing at the next sequence of data (if it exists)
+  self.buffer = null;
+
+  var message = this.getMessage(ber);
+  if (!message)
+    return end();
 
   try {
-    if (this._message === null) {
-      var ber = new BerReader(data);
-      if (!this._newMessage(ber))
-        return false;
-
-      data = data.slice(ber.offset);
-    }
-
-    if (data.length > this._messageLength) {
-      extra = data.slice(ber.length);
-      data = data.slice(0, ber.length);
-    }
-
-    assert.ok(this._message.parse(data, ber.length));
-
-    var message = this._message;
-    this._reset();
+    message.parse(ber);
     this.emit('message', message);
-
   } catch (e) {
-    self.emit('error', e, self._message);
+    this.emit('error', e, message);
     return false;
   }
 
-  // Another message is already there
-  if (extra) {
-    if (this.log.isTraceEnabled())
-      this.log.trace('parsing extra bytes: ' + util.inspect(extra));
-
-    return this.write(extra);
-  }
-
-  return true;
+  return end();
 };
 
 
-Parser.prototype._newMessage = function(ber) {
+Parser.prototype.getMessage = function(ber) {
   assert.ok(ber);
 
-  if (this._messageLength === null) {
-    if (ber.readSequence() === null) { // not enough data for the length?
-      this._buffer = ber.buffer;
-      if (this.log.isTraceEnabled())
-        this.log.trace('Not enough data for the message header');
-
-      return false;
-    }
-    this._messageLength = ber.length;
-
-  }
-
-  if (ber.remain < this._messageLength) {
-    if (this.log.isTraceEnabled())
-      this.log.trace('Not enough data for the message');
-
-    this._buffer = ber.buffer;
-    return false;
-  }
+  var log = this.log;
+  var self = this;
 
   var messageID = ber.readInt();
   var type = ber.readSequence();
 
-  if (this.log.isTraceEnabled())
-    this.log.trace('message id=%d, type=0x%s', messageID, type.toString(16));
-
   var Message;
   switch (type) {
 
@@ -230,28 +206,23 @@ Parser.prototype._newMessage = function(ber) {
     break;
 
   default:
-    var e = new Error('protocolOp 0x' + type.toString(16) + ' not supported');
-    this.emit('error', e, new LDAPResult({
-      messageID: messageID,
-      protocolOp: type
-    }));
-    this._reset();
+    this.emit('error',
+              new Error('protocolOp 0x' +
+                        (type ? type.toString(16) : '??') +
+                        ' not supported'
+                       ),
+              new LDAPResult({
+                messageID: messageID,
+                protocolOp: type || Protocol.LDAP_REP_EXTENSION
+              }));
+
     return false;
   }
-  assert.ok(Message);
 
-  var self = this;
-  this._message = new Message({
+
+  return new Message({
     messageID: messageID,
     log4js: self.log4js
   });
-
-  return true;
 };
 
-
-Parser.prototype._reset = function() {
-  this._message = null;
-  this._messageLength = null;
-  this._buffer = null;
-};

lib/messages/search_entry.js

@@ -140,8 +140,8 @@ SearchEntry.prototype._parse = function(ber) {
 
   this.objectName = ber.readString();
   assert.ok(ber.readSequence());
-  var end = ber.offset + ber.length;
 
+  var end = ber.offset + ber.length;
   while (ber.offset < end) {
     var a = new Attribute();
     a.parse(ber);

lib/server.js

@@ -395,14 +395,17 @@ function Server(options) {
       log.error('Exception happened parsing for %s: %s',
                 c.ldap.id, err.stack);
 
+
+      if (!message)
+        return c.destroy();
+
       var res = getResponse(message);
-      if (res) {
-        res.status = 0x02; // protocol error
-        res.errorMessage = err.toString();
-        c.end(res.toBer());
-      } else {
-        c.destroy();
-      }
+      if (!res)
+        return c.destroy();
+
+      res.status = 0x02; // protocol error
+      res.errorMessage = err.toString();
+      c.end(res.toBer());
     });
 
     c.on('data', function(data) {
@@ -804,7 +807,7 @@ Server.prototype._getHandlerChain = function(req) {
     handlers: [(req.protocolOp !== Protocol.LDAP_REQ_EXTENSION ?
                 noSuffixHandler : noExOpHandler)]
   };
-}
+};
 
 
 Server.prototype._mount = function(op, name, argv, notDN) {

package.json

@@ -3,7 +3,7 @@
   "name": "ldapjs",
   "homepage": "http://ldapjs.org",
   "description": "LDAP client and server APIs",
-  "version": "0.2.8",
+  "version": "0.3.0",
   "repository": {
     "type": "git",
     "url": "git://github.com/mcavage/node-ldapjs.git"

tst/client.test.js

@@ -128,7 +128,8 @@ test('setup', function(t) {
       reconnect: false // turn this off for unit testing
     });
     t.ok(client);
-    client.log4js.setLevel('Trace');
+    // client.log4js.setLevel('Trace');
+    // server.log4js.setLevel('Trace');
     t.end();
   });
 

tst/filters/ext.test.js

@@ -34,7 +34,7 @@ test('Construct no args', function(t) {
 test('Construct args', function(t) {
   var f = new ExtensibleFilter({
     matchType: 'foo',
-    value: 'bar',
+    value: 'bar'
   });
   t.ok(f);
   t.equal(f.matchType, 'foo');

tst/messages/del_request.test.js

@@ -46,7 +46,7 @@ test('parse', function(t) {
   var req = new DeleteRequest();
   var reader = new BerReader(ber.buffer);
   reader.readSequence(0x4a);
-  t.ok(req.parse(reader.buffer, reader.length));
+  t.ok(req.parse(reader, reader.length));
   t.equal(req.dn.toString(), 'cn=test');
   t.end();
 });