Skip to content

LOC-7325: stop uncatchable TypeError on empty binary output in Local.start #182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vivianludrick wants to merge 6 commits into
base: master
Choose a base branch
from
Open

LOC-7325: stop uncatchable TypeError on empty binary output in Local.start #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
909dc67
LOC-7325: stop uncatchable TypeError on empty binary output in Local.…
vivianludrick Aug 21, 2026
1db2f0b
LOC-7325: close remaining escape paths in binary output handling
vivianludrick Aug 31, 2026
7cbe054
LOC-7325: harden adjacent callback paths from second review round
vivianludrick Aug 31, 2026
b9fe94e
LOC-7325: third review round — download-flow hardening + payload sema…
vivianludrick Aug 31, 2026
69bd942
LOC-7325: fourth review round — close all six remaining findings
vivianludrick Sep 1, 2026
c97a9d1
LOC-7325: make the LocalBinary Download tests actually run
vivianludrick Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@ declare module "browserstack-local" {
[key: string]: string | boolean;
}

interface LocalError extends Error {
/** Raw binary output (truncated to 1KB) attached when the output could not be parsed. */
extra?: string;
}

class Local {
start(options: Partial<Options>, callback: (error?: Error) => void): void;
start(options: Partial<Options>, callback: (error?: LocalError) => void): void;
startSync(options: Partial<Options>): LocalError | undefined;
isRunning(): boolean;
stop(callback: () => void): void;
stop(callback: (error?: LocalError) => void): void;
}
}
247 changes: 200 additions & 47 deletions lib/Local.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ function Local(){
this.windows = os.platform().match(/mswin|msys|mingw|cygwin|bccwin|wince|emc|win32/i);
this.pid = undefined;
this.isProcessRunning = false;
this.userProvidedBinaryPath = false;
this.retriesLeft = 9;
this.key = process.env.BROWSERSTACK_ACCESS_KEY;
this.logfile = this.sanitizePath(path.join(process.cwd(), 'local.log'));
Expand All @@ -35,7 +36,15 @@ function Local(){
if(typeof options['onlyCommand'] !== 'undefined')
return;

const binaryPath = this.getBinaryPath(null, options['bs-host']);
var binaryPath;
try {
binaryPath = this.getBinaryPath(null, options['bs-host']);
} catch(err) {
// getAvailableDirs() throws when none of the candidate directories is
// writable (locked-down CI containers). Report it the way startSync
// reports every other failure instead of throwing at the caller.
return new LocalError(err.toString());
}
that.binaryPath = binaryPath;
try {
fs.writeFileSync(that.logfile, '');
Expand All @@ -48,16 +57,21 @@ function Local(){
}
try{
const obj = childProcess.spawnSync(that.binaryPath, that.getBinaryArgs());
if(obj.error)
throw obj.error;
this.tunnel = {pid: obj.pid};
var data = {};
if(obj.stdout.length > 0)
data = JSON.parse(obj.stdout);
else if(obj.stderr.length > 0)
data = JSON.parse(obj.stderr);
else
return new LocalError('No output received');
var result = that.parseBinaryOutput(obj.stdout, obj.stderr);
if(result.error) {
if(result.invalidOutput) {
// A cached binary that runs but prints garbage may be corrupt on
// disk; evict it so the next start re-downloads a fresh copy.
that.evictDownloadedBinary();
}
return result.error;
}
var data = result.data;
if(data['state'] != 'connected'){
return new LocalError(data['message']['message']);
return new LocalError(that.getErrorMessage(data));
} else {
that.pid = data['pid'];
that.isProcessRunning = true;
Expand All @@ -66,13 +80,8 @@ function Local(){
}catch(error){
const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
console.error(binaryDownloadErrorMessage);
if(that.retriesLeft > 0) {
console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
that.retriesLeft -= 1;
fs.unlinkSync(that.binaryPath);
delete(that.binaryPath);
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
if(that.shouldRetryBinaryDownload()) {
that.prepareBinaryRetry(binaryDownloadErrorMessage);
return that.startSync(options);
} else {
throw new LocalError(error.toString());
Expand All @@ -88,7 +97,24 @@ function Local(){
if(typeof options['onlyCommand'] !== 'undefined')
return callback();

this.getBinaryPath(function(binaryPath){
// Every path below must deliver exactly one callback. The execFile handler
// runs inside node's exithandler (a throw there is an uncaughtException the
// caller cannot catch), and getBinaryPath can throw synchronously before
// any of it runs. See LOC-7325.
var settled = false;
var deliver = function(err){
if(settled) return;
settled = true;
callback(err);
};

var onBinaryPath = function(binaryPath){
if(!binaryPath){
// Terminal download failure signalled by LocalBinary (falsy path);
// retrying here would only re-run the whole download cascade.
var downloadErrorMessage = (that.binary && that.binary.downloadErrorMessage) || 'Unable to download BrowserStack Local binary';
return deliver(new LocalError(downloadErrorMessage));
}
that.binaryPath = binaryPath;
try {
fs.writeFileSync(that.logfile, '');
Expand All @@ -98,40 +124,163 @@ function Local(){

that.opcode = 'start';
that.tunnel = childProcess.execFile(that.binaryPath, that.getBinaryArgs(), function(error, stdout, stderr){
if(error) {
const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
console.error(binaryDownloadErrorMessage);
if(that.retriesLeft > 0) {
console.log('Retrying Binary Download. Retries Left', that.retriesLeft);
that.retriesLeft -= 1;
fs.unlinkSync(that.binaryPath);
delete(that.binaryPath);
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
that.start(options, callback);
try {
var result = that.parseBinaryOutput(stdout, stderr);
if(error) {
const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
console.error(binaryDownloadErrorMessage);
if(result.data) {
// The binary executed and reported a structured result, so the
// failure is not a corrupt download — retrying (delete +
// re-download) cannot help; fail fast with the richer diagnostic
// instead of burning the retry budget first.
if(result.data['state'] == 'connected' && result.data['pid'] && running(result.data['pid'])) {
// The daemon is genuinely up even though the foreground process
// exited non-zero; treat it as success so isRunning()/stop()
// agree with reality. The liveness check matters: a payload
// claiming 'connected' from a daemon that then died would
// otherwise be reported as a successful start, and the caller
// would run its whole suite against a dead tunnel.
that.pid = result.data['pid'];
that.isProcessRunning = true;
deliver();
return;
}
var payloadMessage = that.extractErrorMessage(result.data);
if(payloadMessage) {
deliver(new LocalError(payloadMessage));
return;
}
// Payload parsed but carries no usable message: surface the
// exec error and keep the raw payload as extra.
var rawPayload = (stdout && stdout.length > 0) ? stdout : stderr;
deliver(new LocalError(error.toString(), that.truncateForExtra(rawPayload)));
return;
}
if(that.shouldRetryBinaryDownload()) {
that.prepareBinaryRetry(binaryDownloadErrorMessage);
that.start(options, callback);
return;
}
// Keep any raw (non-JSON) output as extra — it usually holds
// the crash text.
deliver(new LocalError(error.toString(), result.error && result.error.extra));
return;
}

if(result.error) {
if(result.invalidOutput) {
// A cached binary that runs but prints garbage may be corrupt
// on disk; evict it so the next start re-downloads a fresh copy.
that.evictDownloadedBinary();
}
deliver(result.error);
return;
}
if(result.data['state'] != 'connected'){
deliver(new LocalError(that.getErrorMessage(result.data)));
} else {
callback(new LocalError(error.toString()));
that.pid = result.data['pid'];
that.isProcessRunning = true;
deliver();
}
} catch(err) {
if(settled) throw err; // the caller's own callback threw — theirs to handle
deliver(new LocalError(err.toString()));
}
});
};

var data = {};
if(stdout)
data = JSON.parse(stdout);
else if(stderr)
data = JSON.parse(stderr);
else
callback(new LocalError('No output received'));
try {
this.getBinaryPath(onBinaryPath, options['bs-host']);
} catch(err) {
// Same synchronous getAvailableDirs() throw as startSync. Without this
// it escapes start() and the callback never fires at all.
deliver(new LocalError(err.toString()));
}
};

if(data['state'] != 'connected'){
callback(new LocalError(data['message']['message']));
} else {
that.pid = data['pid'];
that.isProcessRunning = true;
callback();
}
});
}, options['bs-host']);
// The binary reports failures as {"state": "...", "message": {"message": "..."}},
// but not every non-connected payload carries a message key, and the value is
// not guaranteed to be a string. Dereferencing it blindly throws, and inside
// the execFile callback that throw is an uncaughtException the caller cannot
// catch; a non-string message crashes consumers doing error.message.match().
// See LOC-7325.
this.extractErrorMessage = function(data){
var message = data && data['message'];
if(message && typeof message === 'object')
message = message['message'];
if(typeof message === 'string' && message.length > 0)
return message;
return null;
};

this.getErrorMessage = function(data){
return this.extractErrorMessage(data) || 'Failed to start BrowserStack Local';
};

// Shared retry bookkeeping for start and startSync: drop the (possibly
// corrupt) binary so the next attempt re-downloads it, and record the
// failure for the fallback download source.
this.prepareBinaryRetry = function(binaryDownloadErrorMessage){
console.log('Retrying Binary Download. Retries Left', this.retriesLeft);
this.retriesLeft -= 1;
// Shares evictDownloadedBinary so both deletion paths honour the same
// "never delete a user-supplied binary" rule.
this.evictDownloadedBinary();
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
};

// Re-downloading only makes sense for a binary we downloaded ourselves.
// addArgs re-applies the `binarypath` option on every retry, so retrying a
// user-supplied binary would re-exec the file we just deleted, burn the
// whole retry budget, and replace the real diagnostic with ENOENT.
// See LOC-7325.
this.shouldRetryBinaryDownload = function(){
return this.retriesLeft > 0 && !this.userProvidedBinaryPath;
};

// Raw binary output can be up to execFile's 1MB maxBuffer; truncate before
// attaching it to an error so serializers don't dump the whole buffer.
this.truncateForExtra = function(output){
output = String(output);
if(output.length <= 1024)
return output;
return output.slice(0, 1024) + ' [truncated ' + (output.length - 1024) + ' bytes]';
};

// Shared by start and startSync so both classify binary output the same way.
// Returns {data} for a parsed JSON object, {error} otherwise — including
// output that parses to null or a non-object ('null' is valid JSON, so a
// parse guard alone does not cover it).
this.parseBinaryOutput = function(stdout, stderr){
var output = (stdout && stdout.length > 0) ? stdout : stderr;
if(!output || output.length === 0)
return { error: new LocalError('No output received') };
var data;
try {
data = JSON.parse(output);
} catch(parseError) {
return { error: new LocalError('Invalid output received: ' + parseError.message, this.truncateForExtra(output)), invalidOutput: true };
}
if(!data || typeof data !== 'object')
return { error: new LocalError('Invalid output received: expected a JSON object', this.truncateForExtra(output)), invalidOutput: true };
return { data: data };
};

// A binary that executes but prints unparseable output may be corrupt on
// disk; evicting it lets the next start() download a fresh copy instead of
// failing identically forever. Binaries supplied by the user via the
// `binarypath` option are never evicted.
this.evictDownloadedBinary = function(){
if(this.userProvidedBinaryPath || !this.binaryPath) return;
try {
fs.unlinkSync(this.binaryPath);
} catch(unlinkError) {
console.error('Could not delete binary: ', unlinkError.message);
}
delete(this.binaryPath);
};

this.isRunning = function(){
Expand All @@ -141,7 +290,9 @@ function Local(){
this.stop = function (callback) {
if(!this.pid) return callback();
this.killAllProcesses(function(error){
if(error) callback(new LocalError(error.toString()));
// Without the return, a treeKill error fired the callback twice:
// once with the error, then once with undefined.
if(error) return callback(new LocalError(error.toString()));
callback();
});
};
Expand Down Expand Up @@ -241,8 +392,10 @@ function Local(){
break;

case 'binarypath':
if(value)
if(value){
this.binaryPath = value;
this.userProvidedBinaryPath = true;
}
break;

default:
Expand Down
Loading
Loading