[tbb-commits] [torbutton/master] Bug 40027: Make torbutton_send_ctrl_cmd async

sysrqb at torproject.org sysrqb at torproject.org
Fri Aug 27 20:52:54 UTC 2021


commit ba03ae5f2ec1ce5c9fd46e4063b477fc8626f2d1
Author: Alex Catarineu <acat at torproject.org>
Date:   Tue Jan 26 10:39:41 2021 +0100

    Bug 40027: Make torbutton_send_ctrl_cmd async
    
    - makes torbutton_send_ctrl_cmd async; now implemented using
      torController.sendCommand rather than handling all of the io,
      concurrency, and syncrhonization
    - insert asyncs/awaits/catch to the relevant places where
      torbutton_send_ctrl_cmd are called
    - removed call to torbutton_do_tor_check() from
      torbutton_new_window()
    - NS_BASE_STREAM_CLOSED returned from read no longer triggers error
      callback inside io.pumpInputStream
    - tor.controllerCache is now expicitly a JavaScript map object
    - controller() function now has option to always get a new
      tor.controller, instead of a cached one
    - refactored controller() function to use explicit map() functions
---
 chrome/content/torbutton.js | 105 +++++++++++++-------------------------------
 modules/tor-control-port.js |  52 +++++++++++++++-------
 2 files changed, 66 insertions(+), 91 deletions(-)

diff --git a/chrome/content/torbutton.js b/chrome/content/torbutton.js
index 48539a96..8c016b39 100644
--- a/chrome/content/torbutton.js
+++ b/chrome/content/torbutton.js
@@ -28,7 +28,7 @@ let {
   torbutton_log,
   torbutton_get_property_string,
 } = ChromeUtils.import("resource://torbutton/modules/utils.js", {});
-let { configureControlPortModule } = Cu.import("resource://torbutton/modules/tor-control-port.js", {});
+let { configureControlPortModule, controller } = Cu.import("resource://torbutton/modules/tor-control-port.js", {});
 
 const k_tb_tor_check_failed_topic = "Torbutton:TorCheckFailed";
 
@@ -299,19 +299,19 @@ torbutton_init = function() {
 
 var torbutton_abouttor_message_handler = {
   // Receive IPC messages from the about:tor content script.
-  receiveMessage: function(aMessage) {
+  receiveMessage: async function(aMessage) {
     switch(aMessage.name) {
       case "AboutTor:Loaded":
         aMessage.target.messageManager.sendAsyncMessage("AboutTor:ChromeData",
-                                                    this.getChromeData(true));
+                                                    await this.getChromeData(true));
         break;
     }
   },
 
   // Send privileged data to all of the about:tor content scripts.
-  updateAllOpenPages: function() {
+  updateAllOpenPages: async function() {
     window.messageManager.broadcastAsyncMessage("AboutTor:ChromeData",
-                                                this.getChromeData(false));
+                                                await this.getChromeData(false));
   },
 
   // The chrome data contains all of the data needed by the about:tor
@@ -319,11 +319,11 @@ var torbutton_abouttor_message_handler = {
   // It is sent to the content process when an about:tor window is opened
   // and in response to events such as the browser noticing that Tor is
   // not working.
-  getChromeData: function(aIsRespondingToPageLoad) {
+  getChromeData: async function(aIsRespondingToPageLoad) {
     let dataObj = {
       mobile: torbutton_is_mobile(),
       updateChannel: AppConstants.MOZ_UPDATE_CHANNEL,
-      torOn: torbutton_tor_check_ok()
+      torOn: await torbutton_tor_check_ok()
     };
 
     if (aIsRespondingToPageLoad) {
@@ -446,70 +446,27 @@ function torbutton_array_to_hexdigits(array) {
 
 // Bug 1506 P4: Control port interaction. Needed for New Identity.
 //
-// Executes a command on the control port.
-// Return a string response upon success and null upon error.
-function torbutton_send_ctrl_cmd(command) {
-
-  // We spin the event queue until it is empty and we can be sure that sending
-  // NEWNYM is not leading to a deadlock (see bug 9531 comment 23 for an
-  // invstigation on why and when this may happen). This is surrounded by
-  // suppressing/unsuppressing user initiated events in a window's document to
-  // be sure that these events are not interfering with processing events being
-  // in the event queue.
-  var thread = Services.tm.currentThread;
-  m_tb_domWindowUtils.suppressEventHandling(true);
-  while (thread.processNextEvent(false)) {}
-  m_tb_domWindowUtils.suppressEventHandling(false);
-
+// Asynchronously executes a command on the control port.
+// returns the response as a string, or null on error
+async function torbutton_send_ctrl_cmd(command) {
+  const getErrorMessage = e => (e && (e.torMessage || e.message)) || "";
+  let response = null;
   try {
-    let sts = Cc["@mozilla.org/network/socket-transport-service;1"]
-        .getService(Ci.nsISocketTransportService);
-    let socket;
-    if (m_tb_control_ipc_file) {
-      socket = sts.createUnixDomainTransport(m_tb_control_ipc_file);
-    } else {
-      socket = sts.createTransport([], m_tb_control_host,
-                                   m_tb_control_port, null);
-    }
-
-    // If we don't get a response from the control port in 2 seconds, someting is wrong..
-    socket.setTimeout(Ci.nsISocketTransport.TIMEOUT_READ_WRITE, 2);
-
-    var input = socket.openInputStream(3, 1, 1); // 3 == OPEN_BLOCKING|OPEN_UNBUFFERED
-    var output = socket.openOutputStream(3, 1, 1); // 3 == OPEN_BLOCKING|OPEN_UNBUFFERED
-
-    var inputStream     = Cc["@mozilla.org/binaryinputstream;1"].createInstance(Ci.nsIBinaryInputStream);
-    var outputStream    = Cc["@mozilla.org/binaryoutputstream;1"].createInstance(Ci.nsIBinaryOutputStream);
+    const avoidCache = true;
+    let torController = controller(e => { throw e; }, avoidCache);
 
-    inputStream.setInputStream(input);
-    outputStream.setOutputStream(output);
-
-    var auth_cmd = "AUTHENTICATE "+m_tb_control_pass+"\r\n";
-    outputStream.writeBytes(auth_cmd, auth_cmd.length);
-
-    var bytes = torbutton_socket_readline(inputStream);
-
-    if (bytes.indexOf("250") != 0) {
-      torbutton_safelog(4, "Unexpected auth response on control port "+m_tb_control_desc+":", bytes);
-      return null;
+    let bytes = await torController.sendCommand(command);
+    if (!bytes.startsWith("250")) {
+      throw `Unexpected command response on control port '${bytes}'`;
     }
+    response = bytes.slice(4);
 
-    outputStream.writeBytes(command, command.length);
-    bytes = torbutton_socket_readline(inputStream);
-    if(bytes.indexOf("250") != 0) {
-      torbutton_safelog(4, "Unexpected command response on control port "+m_tb_control_desc+":", bytes);
-      return null;
-    }
-
-    // Closing these streams prevents a shutdown hang on Mac OS. See bug 10201.
-    inputStream.close();
-    outputStream.close();
-    socket.close(Cr.NS_OK);
-    return bytes.substr(4);
-  } catch(e) {
-    torbutton_log(4, "Exception on control port "+e);
-    return null;
+    torController.close();
+  } catch(err) {
+    let msg = getErrorMessage(err);
+    torbutton_log(4, `Error: ${msg}`);
   }
+  return response;
 }
 
 // Bug 1506 P4: Needed for New IP Address
@@ -800,7 +757,7 @@ async function torbutton_do_new_identity() {
     torbutton_log(5, "Torbutton cannot safely newnym. It does not have access to the Tor Control Port.");
     window.alert(warning);
   } else {
-    if (!torbutton_send_ctrl_cmd("SIGNAL NEWNYM\r\n")) {
+    if (!await torbutton_send_ctrl_cmd("SIGNAL NEWNYM")) {
       var warning = torbutton_get_property_string("torbutton.popup.no_newnym");
       torbutton_log(5, "Torbutton was unable to request a new circuit from Tor");
       window.alert(warning);
@@ -930,7 +887,7 @@ function torbutton_use_nontor_proxy()
   torbutton_do_new_identity();
 }
 
-function torbutton_do_tor_check()
+async function torbutton_do_tor_check()
 {
   let checkSvc = Cc["@torproject.org/torbutton-torCheckService;1"]
                    .getService(Ci.nsISupports).wrappedJSObject;
@@ -948,7 +905,7 @@ function torbutton_do_tor_check()
       !env.exists(kEnvUseTransparentProxy) &&
       !env.exists(kEnvSkipControlPortTest) &&
       m_tb_prefs.getBoolPref("extensions.torbutton.local_tor_check")) {
-    if (torbutton_local_tor_check())
+    if (await torbutton_local_tor_check())
       checkSvc.statusOfTorCheck = checkSvc.kCheckSuccessful;
     else {
       // The check failed.  Update toolbar icon and tooltip.
@@ -961,7 +918,7 @@ function torbutton_do_tor_check()
   }
 }
 
-function torbutton_local_tor_check()
+async function torbutton_local_tor_check()
 {
   let didLogError = false;
 
@@ -972,7 +929,7 @@ function torbutton_local_tor_check()
   // Ask tor for its SOCKS listener address and port and compare to the
   // browser preferences.
   const kCmdArg = "net/listeners/socks";
-  let resp = torbutton_send_ctrl_cmd("GETINFO " + kCmdArg + "\r\n");
+  let resp = await torbutton_send_ctrl_cmd("GETINFO " + kCmdArg);
   if (!resp)
     return false;
 
@@ -1116,9 +1073,9 @@ function torbutton_initiate_remote_tor_check() {
   }
 } // torbutton_initiate_remote_tor_check()
 
-function torbutton_tor_check_ok()
+async function torbutton_tor_check_ok()
 {
-  torbutton_do_tor_check();
+  await torbutton_do_tor_check();
   let checkSvc = Cc["@torproject.org/torbutton-torCheckService;1"]
                    .getService(Ci.nsISupports).wrappedJSObject;
   return (checkSvc.kCheckFailed != checkSvc.statusOfTorCheck);
@@ -1486,8 +1443,6 @@ function torbutton_new_window(event)
       progress.addProgressListener(torbutton_resizelistener,
                                    Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
     }
-
-    torbutton_do_tor_check();
 }
 
 // Bug 1506 P2: This is only needed because we have observers
diff --git a/modules/tor-control-port.js b/modules/tor-control-port.js
index a3e72dd5..7891f0ee 100644
--- a/modules/tor-control-port.js
+++ b/modules/tor-control-port.js
@@ -82,7 +82,9 @@ io.pumpInputStream = function (inputStream, onInputData, onError) {
               onInputData(chunk);
               awaitNextChunk();
             } catch (err) {
-              onError(err);
+              if (err.result !== Cr.NS_BASE_STREAM_CLOSED) {
+                onError(err);
+              }
             }
           }
         }, 0, 0, Services.tm.currentThread);
@@ -285,9 +287,9 @@ io.controlSocket = function (ipcFile, host, port, password, onError) {
   // Pass asynchronous notifications to notification dispatcher.
   mainDispatcher.addCallback(/^650/, notificationDispatcher.pushMessage);
   // Log in to control port.
-  sendCommand("authenticate " + (password || ""));
+  sendCommand("authenticate " + (password || "")).catch(onError);
   // Activate needed events.
-  sendCommand("setevents stream");
+  sendCommand("setevents stream").catch(onError);
   return { close : socket.close, sendCommand : sendCommand,
            addNotificationCallback : notificationDispatcher.addCallback,
            removeNotificationCallback : notificationDispatcher.removeCallback };
@@ -677,7 +679,7 @@ let tor = {};
 // __tor.controllerCache__.
 // A map from "unix:socketpath" or "host:port" to controller objects. Prevents
 // redundant instantiation of control sockets.
-tor.controllerCache = {};
+tor.controllerCache = new Map();
 
 // __tor.controller(ipcFile, host, port, password, onError)__.
 // Creates a tor controller at the given ipcFile or host and port, with the
@@ -697,7 +699,8 @@ tor.controller = function (ipcFile, host, port, password, onError) {
            watchEvent : (type, filter, onData) =>
                           event.watchEvent(socket, type, filter, onData),
            isOpen : () => isOpen,
-           close : () => { isOpen = false; socket.close(); }
+           close : () => { isOpen = false; socket.close(); },
+           sendCommand: cmd => socket.sendCommand(cmd),
          };
 };
 
@@ -732,23 +735,40 @@ var configureControlPortModule = function (ipcFile, host, port, password) {
 //     let replyPromise = c.getInfo("ip-to-country/16.16.16.16");
 //     // Close the controller permanently
 //     c.close();
-var controller = function (onError) {
+var controller = function (onError, avoidCache) {
   if (!controlPortInfo.ipcFile && !controlPortInfo.host)
     throw new Error("Please call configureControlPortModule first");
 
   const dest = (controlPortInfo.ipcFile)
                ? `unix:${controlPortInfo.ipcFile.path}`
                : `${controlPortInfo.host}:${controlPortInfo.port}`;
-  const maybeController = tor.controllerCache[dest];
-  if (maybeController && maybeController.isOpen())
-    return maybeController;
-
-  tor.controllerCache[dest] = tor.controller(controlPortInfo.ipcFile,
-                                             controlPortInfo.host,
-                                             controlPortInfo.port,
-                                             controlPortInfo.password,
-                                             onError);
-  return tor.controllerCache[dest];
+
+  // constructor shorthand
+  const newTorController =
+    () => {
+      return tor.controller(
+        controlPortInfo.ipcFile,
+        controlPortInfo.host,
+        controlPortInfo.port,
+        controlPortInfo.password,
+        onError);
+    };
+
+  // avoid cache so always return a new controller
+  if (avoidCache) {
+    return newTorController();
+  }
+
+  // first check our cache and see if we already have one
+  let cachedController = tor.controllerCache.get(dest);
+  if (cachedController && cachedController.isOpen()) {
+    return cachedController;
+  }
+
+  // create a new one and store in the map
+  cachedController = newTorController();
+  tor.controllerCache.set(dest, cachedController);
+  return cachedController;
 };
 
 // Export functions for external use.





More information about the tbb-commits mailing list