From 0899642844e01661bead3653d4847e2f1ba7d864 Mon Sep 17 00:00:00 2001 From: Taylor `Riastradh' Campbell Date: Tue, 10 Jul 2018 17:40:28 +0000 Subject: [PATCH 1/6] Out of paranoia, check we're closing the right control channel. --- app/tor.js | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/app/tor.js b/app/tor.js index 67786b32a93..a86c8f52ce9 100644 --- a/app/tor.js +++ b/app/tor.js @@ -490,9 +490,10 @@ class TorDaemon extends EventEmitter { const readable = controlSocket const writable = controlSocket - this._control = new TorControl(readable, writable) - this._control.on('error', (err) => this._controlError(err)) - this._control.on('close', () => this._controlClosed()) + const control = new TorControl(readable, writable) + this._control = control + this._control.on('error', (err) => this._controlError(control, err)) + this._control.on('close', () => this._controlClosed(control)) // We have finished polling, _and_ we are scheduled to be // notified either by (a) our file system activity watcher, or @@ -538,12 +539,17 @@ class TorDaemon extends EventEmitter { * socket. Report it to the console and give up by destroying the * control connection. * + * @param {TorControl} control * @param {Error} err */ - _controlError (err) { - assert(this._control) + _controlError (control, err) { + if (this._control === control) { + this._control = null + } else { + console.log('tor: control error got deferred') + } console.log(`tor: control socket error: ${err}`) - this._control.destroy(err) + control.destroy(err) } /* @@ -552,10 +558,15 @@ class TorDaemon extends EventEmitter { * has exited. * * TODO(riastradh): Also try to restart tor or anything? + * + * @param {TorControl} control */ - _controlClosed () { - assert(this._control) - this._control = null + _controlClosed (control) { + if (this._control === control) { + this._control = null + } else { + console.log('tor: control closure got deferred') + } // Assume this means the process exited. this.emit('exit') // Poll in case we received a watch event for file system activity From 2a30a9b1d696dc3afbeb07926ff7568fdb95dd86 Mon Sep 17 00:00:00 2001 From: Taylor `Riastradh' Campbell Date: Tue, 10 Jul 2018 17:44:06 +0000 Subject: [PATCH 2/6] Allow TorControl.destroy to be idempotent. This should address the failure reported in: https://github.com/brave/browser-laptop/pull/14675#issuecomment-403163877 fix #14630 Auditors: @diracdeltas Test Plan: see #14630 --- app/tor.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/tor.js b/app/tor.js index a86c8f52ce9..eabf30b14db 100644 --- a/app/tor.js +++ b/app/tor.js @@ -955,10 +955,14 @@ class TorControl extends EventEmitter { * - Mark the control closed so it can't be used any more. * - Pass an error to all callbacks waiting for command completion. * + * Idempotent. Repeated calls have no effect. + * * @param {Error} err */ destroy (err) { - assert(!this._destroyed) + if (this._destroyed) { + return + } this._readable.destroy(err) this._writable.end() this._writable.destroy(err) From 55e7881ee58e70a967413ab6a6927f57e7fd5b16 Mon Sep 17 00:00:00 2001 From: Taylor `Riastradh' Campbell Date: Tue, 10 Jul 2018 18:23:11 +0000 Subject: [PATCH 3/6] Make TorDaemon.kill() idempotent. This, and TorControl.destroy(), are worth making idempotent because errors may happen synchronously _or_ asynchronously, so there are two levels at which a caller might want to clean up. For example, authentication failure is reported as a 515 error from tor, _and_ the tor daemon will close the control connection leading to an asynchronous error. --- app/tor.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/tor.js b/app/tor.js index eabf30b14db..8d5f19cde7d 100644 --- a/app/tor.js +++ b/app/tor.js @@ -150,8 +150,13 @@ class TorDaemon extends EventEmitter { /** * Kill the tor daemon. + * + * Idempotent. Repeated calls have no effect. */ kill () { + if (this._watcher === null) { + return + } assert(this._watcher) this._watcher.close() this._watcher = null From be5453e3734fa3aab98bb0ee653c7e46ce000855 Mon Sep 17 00:00:00 2001 From: Taylor `Riastradh' Campbell Date: Tue, 10 Jul 2018 18:26:43 +0000 Subject: [PATCH 4/6] Emit an error event if authentication fails. --- app/tor.js | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/app/tor.js b/app/tor.js index 8d5f19cde7d..2b599858be9 100644 --- a/app/tor.js +++ b/app/tor.js @@ -170,6 +170,17 @@ class TorDaemon extends EventEmitter { } } + /** + * Internal subroutine. Report an error and kill the daemon. + * + * @param {string} msg - error message + */ + _error (msg) { + console.log(msg) + this.emit('error', msg) + this.kill() + } + /** * Internal subroutine. Called by fs.watch when the tor watch * directory is changed. If the control port file is newly written, @@ -514,22 +525,16 @@ class TorDaemon extends EventEmitter { } } if (err) { - console.log(`tor: authentication failure: ${err}`) - this.kill() - return + return this._error(`tor: authentication failure: ${err}`) } this._control.getSOCKSListeners((err, listeners) => { if (err) { - console.log(`tor: failed to get socks addresses: ${err}`) - this.kill() - return + return this._error(`tor: failed to get socks addresses: ${err}`) } this._socks_addresses = listeners this._control.getVersion((err, version) => { if (err) { - console.log(`tor: failed to get version: ${err}`) - this.kill() - return + return this._error(`tor: failed to get version: ${err}`) } this._tor_version = version this.emit('launch', this.getSOCKSAddress()) From 7499ae44570f21775b0af4bd18a1d7afe255b9ab Mon Sep 17 00:00:00 2001 From: Taylor `Riastradh' Campbell Date: Tue, 10 Jul 2018 18:28:16 +0000 Subject: [PATCH 5/6] Don't assert that we have closed. There is no way to know exactly when this might happen, alas: readables and writables don't necessarily emit their close events promptly. --- app/tor.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/tor.js b/app/tor.js index 2b599858be9..77c504f3706 100644 --- a/app/tor.js +++ b/app/tor.js @@ -987,7 +987,6 @@ class TorControl extends EventEmitter { const [, callback] = this._cmdq.shift() callback(err, null, null) } - setImmediate(() => assert(this._closing === 0)) } /** From 44d3aba347762a6c1b752c583c23536b96656d6f Mon Sep 17 00:00:00 2001 From: Taylor `Riastradh' Campbell Date: Tue, 10 Jul 2018 18:29:08 +0000 Subject: [PATCH 6/6] Automatically test tor launch errors. --- test/integration/app/torTest.js | 37 +++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/integration/app/torTest.js b/test/integration/app/torTest.js index 38b929ad1ad..1122c6deb22 100644 --- a/test/integration/app/torTest.js +++ b/test/integration/app/torTest.js @@ -313,5 +313,42 @@ describe('tor unit tests', function () { }) }) }) + + it('handles failure gracefully', function (callback) { + torDaemon.setup(() => { + // Spawn a process. + torProcess = spawnTor(torDaemon) + // Wait half a second to give the tor process a head start. + setTimeout(() => { + // Clobber the authentication cookie. + const zero = Buffer.alloc(32, 0) + fs.writeFile(tor.torControlCookiePath(), zero, (err) => { + if (err) { + return callback(err) + } + // Start watching. + torDaemon.start() + // Wait 2sec for error. + const errorTimeout = setTimeout(() => { + assert.fail('timeout on tor error') + }, 2000) + torDaemon.once('error', (err) => { + assert(err.toString().indexOf('Tor error 515:') !== -1) + clearTimeout(errorTimeout) + assert(err) + // Kill the tor process gently and wait for it to exit. + torProcess.kill('SIGTERM') + const timeoutExited = setTimeout(() => { + assert.fail('tor process failed to exit after 2sec') + }, 2000) + torProcess.once('exit', () => { + clearTimeout(timeoutExited) + callback() + }) + }) + }) + }, 500) + }) + }) }) })