From 868c46bde43fa774e32d493f35284b4e21ded7a5 Mon Sep 17 00:00:00 2001 From: Mark Cavage Date: Fri, 11 Nov 2011 10:08:48 -0800 Subject: [PATCH] GH-26 rewrite network parser --- lib/client.js | 23 ++++-- lib/messages/bind_request.js | 4 +- lib/messages/message.js | 11 +-- lib/messages/parser.js | 125 ++++++++++++------------------- lib/messages/search_entry.js | 2 +- lib/server.js | 19 +++-- package.json | 2 +- tst/client.test.js | 3 +- tst/filters/ext.test.js | 2 +- tst/messages/del_request.test.js | 2 +- 10 files changed, 88 insertions(+), 105 deletions(-) diff --git a/lib/client.js b/lib/client.js index 029f527..b885169 100644 --- a/lib/client.js +++ b/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; }; diff --git a/lib/messages/bind_request.js b/lib/messages/bind_request.js index 086ddfb..e07862d 100644 --- a/lib/messages/bind_request.js +++ b/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; }; diff --git a/lib/messages/message.js b/lib/messages/message.js index c01f1ef..26ab987 100644 --- a/lib/messages/message.js +++ b/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) { diff --git a/lib/messages/parser.js b/lib/messages/parser.js index 39b2854..92fa003 100644 --- a/lib/messages/parser.js +++ b/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; -}; diff --git a/lib/messages/search_entry.js b/lib/messages/search_entry.js index 96f7dce..d1364ad 100644 --- a/lib/messages/search_entry.js +++ b/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); diff --git a/lib/server.js b/lib/server.js index 538f6f1..8dd24bb 100644 --- a/lib/server.js +++ b/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) { diff --git a/package.json b/package.json index 96ded9d..83309e0 100644 --- a/package.json +++ b/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" diff --git a/tst/client.test.js b/tst/client.test.js index f0a4ca2..ec69f0d 100644 --- a/tst/client.test.js +++ b/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(); }); diff --git a/tst/filters/ext.test.js b/tst/filters/ext.test.js index e2e7135..c5ce806 100644 --- a/tst/filters/ext.test.js +++ b/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'); diff --git a/tst/messages/del_request.test.js b/tst/messages/del_request.test.js index e76de76..8e98f0f 100644 --- a/tst/messages/del_request.test.js +++ b/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(); });