[tor-bugs] #14271 [Applications/Tor Browser]: Make Torbutton work with Unix Domain Socket option

[Tor Bug Tracker & Wiki]

#14271: Make Torbutton work with Unix Domain Socket option
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:  brade
     Type:  enhancement                          |         Status:
                                                 |  needs_review
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-torbutton, tbb-security,         |  Actual Points:
  TorBrowserTeam201609R                          |
Parent ID:  #14270                               |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  SponsorU
-------------------------------------------------+-------------------------

Comment (by arthuredelstein):

 Replying to [comment:15 mcs]:
 > Replying to [comment:14 gk]:
 > > I think we should not set `m_tb_control_port` to `9151` now in the
 catch clause and open a new ticket for implementing a saner solution
 across all Torbutton code. We can discuss there what we want that solution
 to be.
 >
 > That sounds like a good plan. I opened #20057.
 > Here is a revised patch for this ticket:
 >
 https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-02&id=abb01118a8445c541bb359d85fe9af941536b43a

 Sorry for the late review. This patch looks good to me -- nice work! I
 have a couple of minor stylistic suggestions that you may or may not want
 to adopt:
 {{{
 +// given socketFile or host and port.
 +io.asyncSocketStreams = function (socketFile, host, port) {
 +  let socketTransport;
 +  let sts = Cc["@mozilla.org/network/socket-transport-service;1"]
 +
 .getService(Components.interfaces.nsISocketTransportService),
 +         UNBUFFERED = Ci.nsITransport.OPEN_UNBUFFERED;
 +
 +  // Create an instance of a socket transport.
 +  if (socketFile) {
 +    socketTransport = sts.createUnixDomainTransport(socketFile);
 +  } else {
 +    socketTransport = sts.createTransport(null, 0, host, port, null);
 +  }
 +
 }}}
 Maybe move `let socketTransport` down to after the comment `// Create an
 instance of a socket transport`? Also, in that file, I had used blank
 lines to separate functions, though that's probably a personal
 eccentricity...

 Also, a somewhat bigger suggestion that could be part of this patch or
 left for another time. In `torbutton.js`, we will now have
 {{{
  var m_tb_control_socket_file = null; // Set if using a UNIX domain
 socket.
  var m_tb_control_port = null;        // Set if not using a socket.
  var m_tb_control_host = null;        // Set if not using a socket.
  var m_tb_control_pass = null;
  var m_tb_control_desc = null;
 }}}
 I imagine it might be cleaner to collect these into a single data object
 like
 {{{
 var m_tb_control = { socket_file, host, port, password, descriptor }
 }}}
 Then we could factor out a single factory function from `torbutton_init()`
 that generates this object. And we could use this object as a single
 argument for functions in `tor-control-port.js` and `tor-circuit-
 display.js` as well.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/14271#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online