LOC-7325: fourth review round — close all six remaining findings

Never delete or retry a user-supplied binary (HIGH). The previous round
added `userProvidedBinaryPath` and honoured it in evictDownloadedBinary,
but the sibling path prepareBinaryRetry still unlinked unconditionally,
so a `binarypath` binary that crashed was deleted out from under the
user. addArgs re-applies the option on every retry, so the retry then
re-exec'd the deleted file, burned all 9 attempts and replaced the real
diagnostic with ENOENT. prepareBinaryRetry now delegates to
evictDownloadedBinary so both deletion paths share one rule, and
shouldRetryBinaryDownload stops the retry for user-supplied paths.

Require the reported pid to be alive before calling a non-zero exit a
success. A payload claiming 'connected' from a daemon that had already
died was reported as a successful start, leaving the caller to run its
whole suite against a dead tunnel. `is-running` was already a dependency.

Route synchronous binary-path failures to the callback. getAvailableDirs
throws when no candidate directory is writable (locked-down CI), and that
throw escaped start() with the callback never fired — the same "caller
never hears back" class this ticket set out to close. start() now has a
single delivery guard covering both the sync throw and the execFile
handler; startSync returns the error like every other failure it reports.

Reject any non-200 download response. The >= 400 guard still let a 3xx
through, and since https.get does not follow redirects the redirect body
was written to the binary path, chmodded 0755 and returned as the binary.

Destroy the write stream on response- and request-level download errors.
pipe() unpipes on source error but never ends the destination, so each
retry leaked a write fd and opened a second writer on the same path.

Make the sync retry path synchronous. retryBinaryDownload deleted the
stale binary inside an fs.stat callback, so it returned undefined and
downloadSync discarded the retry's result: startSync reported "Couldn't
find binary file" on the first failure while the retry chain ran on in
the background holding the event loop open.

Tests: adds test/local_binary_download.js (stubs https.get, so no network
or TLS fixture) and extends the start() suite. 9 of the new assertions
fail on the previous commit. Suite excluding the credential-gated
LocalBinary Download block: 54 passing, same 3 pre-existing failures.

The existing connected-payload test asserted success for pid 12345, which
is not a live process; it now reports this process's own pid, with a new
test covering the dead-pid case.

Author: vivianludrick

lib/Local.js

@@ -36,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, '');
@@ -72,7 +80,7 @@ function Local(){
     }catch(error){
       const binaryDownloadErrorMessage = `Error while trying to execute binary: ${util.format(error)}`;
       console.error(binaryDownloadErrorMessage);
-      if(that.retriesLeft > 0) {
+      if(that.shouldRetryBinaryDownload()) {
         that.prepareBinaryRetry(binaryDownloadErrorMessage);
         return that.startSync(options);
       } else {
@@ -89,12 +97,23 @@ 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 callback(new LocalError(downloadErrorMessage));
+        return deliver(new LocalError(downloadErrorMessage));
       }
       that.binaryPath = binaryPath;
       try {
@@ -105,15 +124,6 @@ function Local(){
 
       that.opcode = 'start';
       that.tunnel = childProcess.execFile(that.binaryPath, that.getBinaryArgs(), function(error, stdout, stderr){
-        // Everything below runs inside node's exithandler: a throw here is an
-        // uncaughtException the caller cannot catch, and a fall-through can
-        // invoke the callback twice. Guard both structurally. See LOC-7325.
-        var callbackCalled = false;
-        var safeCallback = function(err){
-          if(callbackCalled) return;
-          callbackCalled = true;
-          callback(err);
-        };
         try {
           var result = that.parseBinaryOutput(stdout, stderr);
           if(error) {
@@ -124,35 +134,37 @@ function Local(){
               // 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']) {
-                // The daemon came up even though the foreground process exited
-                // non-zero; treat it as success so isRunning()/stop() agree
-                // with reality (startSync likewise ignores the exit status
-                // when the payload says connected).
+              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;
-                safeCallback();
+                deliver();
                 return;
               }
               var payloadMessage = that.extractErrorMessage(result.data);
               if(payloadMessage) {
-                safeCallback(new LocalError(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;
-              safeCallback(new LocalError(error.toString(), that.truncateForExtra(rawPayload)));
+              deliver(new LocalError(error.toString(), that.truncateForExtra(rawPayload)));
               return;
             }
-            if(that.retriesLeft > 0) {
+            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.
-            safeCallback(new LocalError(error.toString(), result.error && result.error.extra));
+            deliver(new LocalError(error.toString(), result.error && result.error.extra));
             return;
           }
 
@@ -162,22 +174,30 @@ function Local(){
               // on disk; evict it so the next start re-downloads a fresh copy.
               that.evictDownloadedBinary();
             }
-            safeCallback(result.error);
+            deliver(result.error);
             return;
           }
           if(result.data['state'] != 'connected'){
-            safeCallback(new LocalError(that.getErrorMessage(result.data)));
+            deliver(new LocalError(that.getErrorMessage(result.data)));
           } else {
             that.pid = result.data['pid'];
             that.isProcessRunning = true;
-            safeCallback();
+            deliver();
           }
         } catch(err) {
-          if(callbackCalled) throw err; // the caller's own callback threw — theirs to handle
-          safeCallback(new LocalError(err.toString()));
+          if(settled) throw err; // the caller's own callback threw — theirs to handle
+          deliver(new LocalError(err.toString()));
         }
       });
-    }, options['bs-host']);
+    };
+
+    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()));
+    }
   };
 
   // The binary reports failures as {"state": "...", "message": {"message": "..."}},
@@ -205,18 +225,22 @@ function Local(){
   this.prepareBinaryRetry = function(binaryDownloadErrorMessage){
     console.log('Retrying Binary Download. Retries Left', this.retriesLeft);
     this.retriesLeft -= 1;
-    try {
-      fs.unlinkSync(this.binaryPath);
-    } catch(unlinkError) {
-      // The binary may already be gone (a prior retry or a concurrent
-      // instance); the retry only needs the path cleared for re-download.
-      console.error('Could not delete binary: ', unlinkError.message);
-    }
-    delete(this.binaryPath);
+    // 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){

lib/LocalBinary.js

@@ -131,23 +131,33 @@ function LocalBinary(){
     this.downloadErrorMessage = errorMessagePrefix + ' : ' + errorMessage;
   };
 
+  this.removeBinaryIfPresentSync = function(binaryPath) {
+    try {
+      if(fs.existsSync(binaryPath))
+        fs.unlinkSync(binaryPath);
+    } catch(unlinkError) {
+      // A held handle (AV scan) or permissions can make the delete fail;
+      // the retry will overwrite the file anyway.
+      console.error('Could not delete binary: ', unlinkError.message);
+    }
+  };
+
   this.retryBinaryDownload = function(conf, destParentDir, callback, retries, binaryPath) {
     var that = this;
     if(retries > 0) {
       console.log('Retrying Download. Retries left', retries);
+      if(!callback) {
+        // downloadSync consumes this return value. Deleting via an async
+        // fs.stat callback made retryBinaryDownload return undefined, so
+        // startSync reported "Couldn't find binary file" on the very first
+        // failure while the retry chain kept running in the background and
+        // held the event loop open. See LOC-7325.
+        that.removeBinaryIfPresentSync(binaryPath);
+        return that.downloadSync(conf, destParentDir, retries - 1);
+      }
       fs.stat(binaryPath, function(err) {
-        if(err == null) {
-          try {
-            fs.unlinkSync(binaryPath);
-          } catch(unlinkError) {
-            // A held handle (AV scan) or permissions can make the delete
-            // fail; the retry will overwrite the file anyway.
-            console.error('Could not delete binary: ', unlinkError.message);
-          }
-        }
-        if(!callback) {
-          return that.downloadSync(conf, destParentDir, retries - 1);
-        }
+        if(err == null)
+          that.removeBinaryIfPresentSync(binaryPath);
         that.download(conf, destParentDir, callback, retries - 1);
       });
     } else {
@@ -268,9 +278,13 @@ function LocalBinary(){
       });
 
       https.get(options, function (response) {
-        if (response.statusCode >= 400) {
-          // Without this check, an error body (404/403 HTML) was written to
-          // binaryPath and reported as a successful download.
+        if (response.statusCode !== 200) {
+          // Anything but 200 is not the binary. Without this check an error
+          // body (404/403 HTML) was written to binaryPath and reported as a
+          // successful download; >= 400 alone still let a 3xx through, since
+          // https.get does not follow redirects and would have written the
+          // redirect body itself, chmodded it 0755 and returned it as the
+          // binary. See LOC-7325.
           if(settle()) return;
           response.resume();
           fileStream.destroy();
@@ -301,6 +315,10 @@ function LocalBinary(){
 
         response.on('error', function(err) {
           if(settle()) return;
+          // pipe() unpipes the destination on a source error but never ends
+          // it: without this the write fd leaks and the retry below opens a
+          // second writer on the same path.
+          fileStream.destroy();
           that.binaryDownloadError('Got Error in binary download response', util.format(err));
           that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
         });
@@ -319,6 +337,7 @@ function LocalBinary(){
         });
       }).on('error', function(err) {
         if(settle()) return;
+        fileStream.destroy();
         that.binaryDownloadError('Got Error in binary downloading request', util.format(err));
         that.retryBinaryDownload(conf, destParentDir, callback, retries, binaryPath);
       });