Skip to content

Commit

Permalink
[fix] Prevent WebSocket#close() from triggering an infinite loop (#969)
Browse files Browse the repository at this point in the history 
This prevents `WebSocket.prototype.close()` from triggering an infinite
loop if called from an error listener while connecting.
  • Loading branch information
@lpinca
lpinca committed Jan 14, 2017
1 parent bd41a05 commit ac2dade
Show file tree
Hide file tree
Showing 2 changed files with 107 additions and 97 deletions.
22 changes: 15 additions & 7 deletions lib/WebSocket.js
View file
Expand Up @@ -263,9 +263,12 @@ class WebSocket extends EventEmitter {
close (code, data) {
if (this.readyState === WebSocket.CLOSED) return;
if (this.readyState === WebSocket.CONNECTING) {
this._req.abort();
this.emit('error', new Error('closed before the connection is established'));
return this.finalize(true);
if (this._req && !this._req.aborted) {
this._req.abort();
this.emit('error', new Error('closed before the connection is established'));
this.finalize(true);
}
return;
}

if (this.readyState === WebSocket.CLOSING) {
Expand Down Expand Up @@ -377,9 +380,12 @@ class WebSocket extends EventEmitter {
terminate () {
if (this.readyState === WebSocket.CLOSED) return;
if (this.readyState === WebSocket.CONNECTING) {
this._req.abort();
this.emit('error', new Error('closed before the connection is established'));
return this.finalize(true);
if (this._req && !this._req.aborted) {
this._req.abort();
this.emit('error', new Error('closed before the connection is established'));
this.finalize(true);
}
return;
}

if (this._socket) {
Expand Down Expand Up @@ -631,6 +637,7 @@ function initAsClient (address, protocols, options) {
this._req.on('error', (error) => {
if (this._req.aborted) return;

this._req = null;
this.emit('error', error);
this.finalize(true);
});
Expand All @@ -644,6 +651,8 @@ function initAsClient (address, protocols, options) {
});

this._req.on('upgrade', (res, socket, head) => {
this._req = null;

const digest = crypto.createHash('sha1')
.update(key + GUID, 'binary')
.digest('base64');
Expand Down Expand Up @@ -688,7 +697,6 @@ function initAsClient (address, protocols, options) {
this.extensions[PerMessageDeflate.extensionName] = perMessageDeflate;
}

this._req = null;
this.setSocket(socket, head);
});
}
182 changes: 92 additions & 90 deletions test/WebSocket.test.js
View file
Expand Up @@ -305,77 +305,6 @@ describe('WebSocket', function () {
});

describe('connection establishing', function () {
it('can terminate before connection is established (1/2)', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
ws.terminate();
});
});

it('can terminate before connection is established (2/2)', function (done) {
const wss = new WebSocketServer({
verifyClient: (info, cb) => setTimeout(cb, 300, true),
port: ++port
}, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
setTimeout(() => ws.terminate(), 150);
});
});

it('can close before connection is established (1/2)', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
ws.close(1001);
});
});

it('can close before connection is established (2/2)', function (done) {
const wss = new WebSocketServer({
verifyClient: (info, cb) => setTimeout(cb, 300, true),
port: ++port
}, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
setTimeout(() => ws.close(1001), 150);
});
});

it('can handle error before request is upgraded', function (done) {
// Here, we don't create a server, to guarantee that the connection will
// fail before the request is upgraded
const ws = new WebSocket(`ws://localhost:${++port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', () => done());
});

it('invalid server key is denied', function (done) {
server.createServer(++port, server.handlers.invalidKey, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);
Expand Down Expand Up @@ -956,7 +885,50 @@ describe('WebSocket', function () {
});

describe('#close', function () {
it('without invalid first argument throws exception', function (done) {
it('closes the connection if called while connecting (1/2)', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
ws.close(1001);
});
});

it('closes the connection if called while connecting (2/2)', function (done) {
const wss = new WebSocketServer({
verifyClient: (info, cb) => setTimeout(cb, 300, true),
port: ++port
}, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
setTimeout(() => ws.close(1001), 150);
});
});

it('can be called from an error listener while connecting', function (done) {
const ws = new WebSocket(`ws://localhost:${++port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.code, 'ECONNREFUSED');
ws.close();
ws.on('close', () => done());
});
});

it('throws an error if the first argument is invalid (1/2)', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

Expand All @@ -971,7 +943,7 @@ describe('WebSocket', function () {
});
});

it('without reserved error code 1004 throws exception', function (done) {
it('throws an error if the first argument is invalid (2/2)', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

Expand All @@ -986,7 +958,7 @@ describe('WebSocket', function () {
});
});

it('without message is successfully transmitted to the server', function (done) {
it('works when close reason is not specified', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

Expand All @@ -1000,22 +972,7 @@ describe('WebSocket', function () {
});
});

it('with message is successfully transmitted to the server', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => ws.close(1000, 'some reason'));

srv.on('close', (code, message, flags) => {
assert.ok(flags.masked);
assert.strictEqual(message, 'some reason');
srv.close(done);
ws.terminate();
});
});
});

it('with encoded message is successfully transmitted to the server', function (done) {
it('works when close reason is specified', function (done) {
server.createServer(++port, (srv) => {
const ws = new WebSocket(`ws://localhost:${port}`);

Expand Down Expand Up @@ -1119,6 +1076,51 @@ describe('WebSocket', function () {
});
});

describe('#terminate', function () {
it('closes the connection if called while connecting (1/2)', function (done) {
const wss = new WebSocketServer({ port: ++port }, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
ws.terminate();
});
});

it('closes the connection if called while connecting (2/2)', function (done) {
const wss = new WebSocketServer({
verifyClient: (info, cb) => setTimeout(cb, 300, true),
port: ++port
}, () => {
const ws = new WebSocket(`ws://localhost:${port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.message, 'closed before the connection is established');
ws.on('close', () => wss.close(done));
});
setTimeout(() => ws.terminate(), 150);
});
});

it('can be called from an error listener while connecting', function (done) {
const ws = new WebSocket(`ws://localhost:${++port}`);

ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
ws.on('error', (err) => {
assert.ok(err instanceof Error);
assert.strictEqual(err.code, 'ECONNREFUSED');
ws.terminate();
ws.on('close', () => done());
});
});
});

describe('WHATWG API emulation', function () {
it('should not throw errors when getting and setting', function () {
const listener = () => {};
Expand Down

0 comments on commit ac2dade

Please sign in to comment.