From 909dc67e66a97239d522bf94ed72db46936e3333 Mon Sep 17 00:00:00 2001 From: Vivian Vijay Ludrick <116781909+vivianludrick@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:17:41 +0530 Subject: [PATCH] LOC-7325: stop uncatchable TypeError on empty binary output in Local.start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `start()` handles the binary's output inside an `execFile` callback. The empty-output branch called back with 'No output received' but did not return, so control fell through to `data['message']['message']` on `data = {}`. That threw a TypeError, and because the throw happens inside a callback invoked by node's internal exithandler, no try/catch around `local.start(...)` could intercept it — it surfaced as an uncaughtException in the host process. Three paths reached the same unguarded deref: - empty stdout and stderr (the reported one) — now returns after the callback, so it fires exactly once - the terminal branch of the `error` handler, which also fell through - any non-connected payload with no `message` key Also guards `JSON.parse`: non-JSON output threw a SyntaxError from the same uncatchable position, and is now reported through the callback with the raw output attached as `extra`. `startSync` shared the unguarded deref and now uses the same helper. Its empty-output branch already returned, so it was not exposed to the fall-through. Adds regression tests driving start() with stub binaries for each output shape, asserting the callback fires exactly once and nothing escapes as an uncaughtException. They need no credentials or network. Three of the four fail on master with the TypeError from the ticket. --- lib/Local.js | 31 +++++++-- test/local_start_output_handling.js | 104 ++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 7 deletions(-) create mode 100644 test/local_start_output_handling.js diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..9d482b6 100644 --- a/lib/Local.js +++ b/lib/Local.js @@ -57,7 +57,7 @@ function Local(){ else return new LocalError('No output received'); if(data['state'] != 'connected'){ - return new LocalError(data['message']['message']); + return new LocalError(that.getErrorMessage(data)); } else { that.pid = data['pid']; that.isProcessRunning = true; @@ -112,19 +112,25 @@ function Local(){ return; } else { callback(new LocalError(error.toString())); + return; } } var data = {}; - if(stdout) - data = JSON.parse(stdout); - else if(stderr) - data = JSON.parse(stderr); - else + var output = stdout || stderr; + if(!output) { callback(new LocalError('No output received')); + return; + } + try { + data = JSON.parse(output); + } catch(parseError) { + callback(new LocalError('Invalid output received: ' + parseError.message, output)); + return; + } if(data['state'] != 'connected'){ - callback(new LocalError(data['message']['message'])); + callback(new LocalError(that.getErrorMessage(data))); } else { that.pid = data['pid']; that.isProcessRunning = true; @@ -134,6 +140,17 @@ function Local(){ }, options['bs-host']); }; + // The binary reports failures as {"state": "...", "message": {"message": "..."}}, + // but not every non-connected payload carries a message key. Dereferencing it + // blindly throws, and inside the execFile callback that throw is an + // uncaughtException the caller cannot catch. See LOC-7325. + this.getErrorMessage = function(data){ + var message = data && data['message']; + if(message && typeof message === 'object') + message = message['message']; + return message || 'Failed to start BrowserStack Local'; + }; + this.isRunning = function(){ return this.pid && running(this.pid) && this.isProcessRunning; }; diff --git a/test/local_start_output_handling.js b/test/local_start_output_handling.js new file mode 100644 index 0000000..458759d --- /dev/null +++ b/test/local_start_output_handling.js @@ -0,0 +1,104 @@ +var expect = require('expect.js'), + fs = require('fs'), + os = require('os'), + path = require('path'), + browserstack = require('../index'); + +// Regression tests for LOC-7325. +// +// `Local.start` handles the binary's output inside an `execFile` callback. A +// throw there is raised by node's internal exithandler, so no try/catch around +// `start()` can intercept it — it surfaces as an uncaughtException and the +// blast radius is set by the host process's exception policy. These tests drive +// `start()` with stub binaries that reproduce each output shape and assert the +// callback fires exactly once with an error, and that nothing throws. +// +// Stubs are shell scripts, so these are skipped on Windows. +describe('Local.start output handling', function () { + var stubDir, bsLocal; + + function stub(name, body) { + var stubPath = path.join(stubDir, name); + fs.writeFileSync(stubPath, '#!/bin/sh\n' + body + '\n', { mode: 0o755 }); + return stubPath; + } + + // Drives start() with the given stub and collects every callback invocation + // plus any uncaughtException raised out of the execFile callback. + function run(stubPath, done) { + var calls = [], uncaught = []; + var existing = process.listeners('uncaughtException'); + process.removeAllListeners('uncaughtException'); + process.on('uncaughtException', function (err) { uncaught.push(err); }); + + bsLocal.binaryPath = stubPath; + bsLocal.start({ key: 'dummy-key', localIdentifier: 'loc-7325' }, function (error) { + calls.push(error); + }); + + // Settle past the execFile callback before asserting, so a second + // (throwing) invocation would have happened by now if it were going to. + setTimeout(function () { + process.removeAllListeners('uncaughtException'); + existing.forEach(function (listener) { process.on('uncaughtException', listener); }); + done(calls, uncaught); + }, 1000); + } + + before(function () { + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bs-local-7325-')); + }); + + beforeEach(function () { + bsLocal = new browserstack.Local(); + // Keep the stubs from clobbering ./local.log in the repo root. + bsLocal.logfile = path.join(stubDir, 'local.log'); + }); + + if (os.platform().match(/win32/i)) { + it.skip('skipped on Windows (stub binaries are shell scripts)'); + return; + } + + it('reports an error exactly once when the binary exits with no output', function (done) { + this.timeout(10000); + run(stub('empty-output.sh', 'exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0]).to.be.an('object'); + expect(calls[0].message).to.equal('No output received'); + done(); + }); + }); + + it('reports an error exactly once when the binary emits non-JSON output', function (done) { + this.timeout(10000); + run(stub('garbage-output.sh', 'echo "segmentation fault"; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.match(/^Invalid output received: /); + expect(calls[0].extra).to.match(/segmentation fault/); + done(); + }); + }); + + it('reports a fallback message when a non-connected payload has no message key', function (done) { + this.timeout(10000); + run(stub('no-message-key.sh', 'echo \'{"state":"disconnected"}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Failed to start BrowserStack Local'); + done(); + }); + }); + + it('surfaces the binary message when a non-connected payload carries one', function (done) { + this.timeout(10000); + run(stub('with-message.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Invalid key'); + done(); + }); + }); +});