Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20111 closed defect (fixed)

use Unix domain sockets for SOCKS port by default

Reported by: mcs Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, tbb-security, tbb-sandboxing, TorBrowserTeam201610R
Cc: brade, gk, arthuredelstein, adrelanos Actual Points:
Parent ID: #14270 Points:
Reviewer: Sponsor:

Description

We should use Unix domain sockets by default in Tor Browser. The patch for #14272 takes care of doing that for the control port (via the extensions.torlauncher.control_port_use_socket = true default preference). For the SOCKS port we need additional changes in tor-browser and builders/tor-browser-bundle at least.

Child Tickets

Attachments (1)

tb6.5a3-quit-log.txt (3.1 KB) - added by mcs 3 years ago.
TB 6.5a3 log (quit on OSX while cnn.com is loading)

Download all attachments as: .zip

Change History (32)

comment:1 Changed 3 years ago by mcs

Cc: arthuredelstein added
Status: newneeds_information

Kathy and I looked at this and soon realized that we cannot simply modify torrc-defaults (SocksPort) and the browser default preferences (network.proxy.socks) to use a socket path. This approach will not work because we need to insert a full path, and we do not know what the path is until the browser is starting up (it won't work to use a relative path, at least not on OSX where the TorBrowser-Data directory may be located in one of two different locations). Possibly solutions:

  • Modify Tor Launcher to Do The Right Thing and configure tor and the browser correctly. This is how we handled the ControlPort.
  • Modify the browser and tor (possibly with help from Tor Launcher) so we can use a placeholder within network.proxy.socks and SocksPort, e.g., network.proxy.socks="file:///--PROFILEDIR--/../../Tor/socks.socket"

Comments? Do you have a better idea? Kathy and I favor the first approach.

comment:2 in reply to:  1 Changed 3 years ago by arthuredelstein

Replying to mcs:

Kathy and I looked at this and soon realized that we cannot simply modify torrc-defaults (SocksPort) and the browser default preferences (network.proxy.socks) to use a socket path. This approach will not work because we need to insert a full path, and we do not know what the path is until the browser is starting up (it won't work to use a relative path, at least not on OSX where the TorBrowser-Data directory may be located in one of two different locations). Possibly solutions:

  • Modify Tor Launcher to Do The Right Thing and configure tor and the browser correctly. This is how we handled the ControlPort.
  • Modify the browser and tor (possibly with help from Tor Launcher) so we can use a placeholder within network.proxy.socks and SocksPort, e.g., network.proxy.socks="file:///--PROFILEDIR--/../../Tor/socks.socket"

Comments? Do you have a better idea? Kathy and I favor the first approach.

To use a ControlPort via a domain socket, Tor Launcher will need to specify the ControlPort socket path as a command argument, correct? So the first approach sounds pretty natural and simple to me, where you also specify a path for the SocksPort and then set the network.proxy.socks pref in the browser.

comment:3 Changed 3 years ago by gk

Doing the Right Thing sounds good to me as well.

comment:4 Changed 3 years ago by adrelanos

Cc: adrelanos added

comment:5 Changed 3 years ago by adrelanos

What will be the name of the environment variable to define the path to the socket file? TOR_SOCKS_SOCKET?

(For the Tor ControlPort it is TOR_CONTROL_SOCKET.) (I am asking for Whonix integration purposes.)

comment:6 in reply to:  5 Changed 3 years ago by mcs

Replying to adrelanos:

What will be the name of the environment variable to define the path to the socket file? TOR_SOCKS_SOCKET?

Yes, we will probably use TOR_SOCKS_SOCKET. We have not created a patch yet though, so suggestions are welcome. We will also add a Boolean preference which will be true by default, maybe extensions.torlauncher.socks_port_use_socket

comment:7 Changed 3 years ago by mcs

See #18753 for a related tor ticket (Unix socket paths cannot contain spaces).

comment:8 Changed 3 years ago by mcs

Keywords: tbb-torbutton tbb-security TorBrowserTeam201609R added
Status: needs_informationneeds_review

This requires two patches, one for Tor Launcher and one for Torbutton:

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20111-01&id=f221ec51c347a354bd58a6b8bb628f29dee1e284

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug20111-01&id=9f6332a4aeb1d3b1a575cebba95903573aab11b4

Kathy and I propose that we leave the browser defaults as-is. Inside tor-browser/browser/app/profile/000-tor-browser.js, we have:

pref("network.proxy.socks", "127.0.0.1");
pref("network.proxy.socks_port", 9150);

That means that a TCP SOCKS port will be used if Tor Launcher is not installed, which seems okay since we do not know what path to use for the socket.

comment:9 Changed 3 years ago by gk

Status: needs_reviewneeds_information

Looking at the Tor Launcher changes I got puzzled by:

+        let socksPort = TorLauncherUtil.getIntPref("network.proxy.socks_port",
+                                                    9500);

Where does the 9500 come from? I somehow expected 9150 there...

comment:10 Changed 3 years ago by brade

Doh! Mark's fingers kept typing 9500 when we were working on this. I had him fix most of them as we worked but apparently I missed one. Sorry!

Do you want a new patch?

comment:11 Changed 3 years ago by gk

Yes, please.

comment:12 Changed 3 years ago by mcs

Status: needs_informationneeds_review

Sorry about my error! Here is a revised patch (we can simply pass 0 because the next line will take care of substituting 9150):

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20111-02&id=1c3056ca164581eb19065743e209b69797c7022b

comment:13 Changed 3 years ago by gk

Status: needs_reviewneeds_information

When testing your patches in a clean, new hardened build I got the following issue:

Sep 28 11:34:45.000 [warn] tor_bug_occurred_(): Bug: src/common/address.c:1119: tor_addr_compare_masked: This line should not have been reached. (Future instances of this warning will be silenced.) (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug: Line unexpectedly reached at tor_addr_compare_masked at src/common/address.c:1119. Stack trace: (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/libasan.so.2(+0x4bc88) [0x7fa4d26eac88] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(log_backtrace+0x46) [0x560e0b946df6] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(tor_bug_occurred_+0x13b) [0x560e0b99423b] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(tor_addr_compare_masked+0x455) [0x560e0b941105] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(connection_edge_compatible_with_circuit+0x2a3) [0x560e0b866873] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(+0x73a276) [0x560e0b7d5276] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(+0x749a3a) [0x560e0b7e4a3a] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(connection_ap_handshake_attach_circuit+0x722) [0x560e0b7e6e22] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(connection_ap_attach_pending+0x4ac) [0x560e0b85707c] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(circuit_build_needed_circs+0xe7) [0x560e0b7e38a7] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(+0x552488) [0x560e0b5ed488] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/libevent-2.0.so.5(event_base_loop+0x937) [0x7fa4d1f488d7] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(do_main_loop+0x398) [0x560e0b5ee898] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(tor_main+0x140d) [0x560e0b5f3a3d] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(main+0x1c) [0x560e0b5e102c] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7fa4d0195700] (on Tor 0.2.9.2-alpha 00ec701f8343f552)
Sep 28 11:34:45.000 [warn] Bug:     /home/thomas/Arbeit/Tor/debugging/20111/tor-browser/Browser/TorBrowser/Tor/tor(+0x547dcd) [0x560e0b5e2dcd] (on Tor 0.2.9.2-alpha 00ec701f8343f552)

Do you see the same on your machines? It seems we are hitting a tor bug or is there something wrong with your patches (they looked reasonable to me after the first code-review pass).

comment:14 in reply to:  13 Changed 3 years ago by mcs

Replying to gk:

Do you see the same on your machines? It seems we are hitting a tor bug or is there something wrong with your patches (they looked reasonable to me after the first code-review pass).

I see the same messages, although I do not remember seeing them before (maybe Kathy and I failed to notice them or maybe we tested with a different version or tor). This could be a tor bug; the tor_addr_compare_masked() function does not seem to support Unix domain sockets (but I have not had time to debug it yet).

comment:15 Changed 3 years ago by gk

Yes, and tor's a885271c08d2337b35c203c0b27509d0aa32dbf6 made it just visible... Want to file a bug against tor with the tbb-needs keyword once you debugged the issue?

comment:16 in reply to:  15 Changed 3 years ago by mcs

Replying to gk:

Yes, and tor's a885271c08d2337b35c203c0b27509d0aa32dbf6 made it just visible... Want to file a bug against tor with the tbb-needs keyword once you debugged the issue?

I created #20261. I also talked to Yawning and teor about this issue.

comment:17 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201609R removed

Moving review tickets to October.

comment:18 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed
Status: needs_informationneeds_revision

Kathy and I are going to revise these patches to account for the #18753 fix.

comment:19 Changed 3 years ago by mcs

Keywords: tbb-sandboxing TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: needs_revisionneeds_review

Here are revised patches for Tor Launcher and Torbutton:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20111-03&id=6ac80cecfc53371791c5b492b22e280abb8ca181

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug20111-02&id=6d36b290c8064b19eb9b931f137131bc9cc47529

We had to include a copy of Tor Launcher's string unescape code in Torbutton, which is a little annoying but seems like the best alternative for now (eventually, we should make it so everything uses Arthur's tor-control-port.js module or its successor).

#20261 is still an outstanding problem.

comment:20 Changed 3 years ago by arthuredelstein

A few very minor comments:

  • "extensions.torlauncher.socks_port_use_socket" is a confusing name to me, because we are using a TCP socket when it is false. Maybe call it "socks_port_use_ipc", as this would also cover the named pipe scenario if we later get it working in Windows? Similar for the env var TOR_SOCKS_SOCKET.
  • _strUnescape: It would be nicer if this function simply returned the unescaped string, and threw an exception if it failed. (Although maybe you want to keep it this way to be consistent with the other copy in Tor launcher.)
  • unescapeTorString: I would suggest defining this after _strUnescape is defined in utils.js, because it refers to it.

Otherwise looks good to me.

comment:21 in reply to:  20 Changed 3 years ago by mcs

Replying to arthuredelstein:

A few very minor comments:

  • "extensions.torlauncher.socks_port_use_socket" is a confusing name to me, because we are using a TCP socket when it is false. Maybe call it "socks_port_use_ipc", as this would also cover the named pipe scenario if we later get it working in Windows? Similar for the env var TOR_SOCKS_SOCKET.

Good point. Kathy and I cleaned this up. We renamed the env vars and preferences as well as various functions and variables. The changes are a little messy, but they seem worthwhile.

  • _strUnescape: It would be nicer if this function simply returned the unescaped string, and threw an exception if it failed. (Although maybe you want to keep it this way to be consistent with the other copy in Tor launcher.)

Done. While testing, we also found some errors in the implementation (oops).

  • unescapeTorString: I would suggest defining this after _strUnescape is defined in utils.js, because it refers to it.

Done.

Revised patches for Tor Launcher and Torbutton:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20111-04&id=e0e8798adf793b1a369430292b881bf3cbbd6693
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug20111-03&id=d66fa277a83b4a117669f6ee8e86591b2988ec22

comment:22 Changed 3 years ago by gk

After testing a bit it seems to me that we can't shut down the browser by clicking on the "x" in the upper right corner. At least not while I am loading a page. This is what I get before I have to kill Tor Browser if I e.g. go to cnn.com:

[10-18 13:13:43] TorLauncher NOTE: Disconnecting from tor process (pid 9679)
Oct 18 15:13:43.000 [notice] Owning controller connection has closed -- exiting now.
Oct 18 15:13:43.000 [notice] Catching signal TERM, exiting cleanly.
[10-18 13:13:43] Torbutton INFO: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [mozIThirdPartyUtil.getFirstPartyURIFromChannel]
[10-18 13:13:43] Torbutton INFO: tor SOCKS isolation catchall: http://edition.i.cdn.cnn.com/.a/fonts/cnn/3.0.0/cnnsans-lightit.woff via --unknown--:a5401a33241f0f5b727475faebb590bf
[10-18 13:13:43] Torbutton INFO: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [mozIThirdPartyUtil.getFirstPartyURIFromChannel]
[10-18 13:13:43] Torbutton INFO: tor SOCKS isolation catchall: http://edition.i.cdn.cnn.com/.a/fonts/cnn/3.0.0/cnnsans-regular.woff via --unknown--:a5401a33241f0f5b727475faebb590bf
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 99 NEW 0 pixel.quantserve.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 100 NEW 0 flapi1.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 101 NEW 0 flapi2.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 102 NEW 0 optimized-by.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 103 NEW 0 flapi1.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 104 NEW 0 flapi2.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 105 NEW 0 flapi1.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 106 NEW 0 flapi2.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 107 NEW 0 optimized-by.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 108 NEW 0 flapi1.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 109 NEW 0 flapi2.rubiconproject.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 39 CLOSED 6 104.16.26.190:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 106 CLOSED 0 flapi2.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 103 CLOSED 0 flapi1.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 98 CLOSED 0 d2lv4zbk7v5f93.cloudfront.net:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 104 CLOSED 0 flapi2.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 100 CLOSED 0 flapi1.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 101 CLOSED 0 flapi2.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 107 CLOSED 0 optimized-by.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 108 CLOSED 0 flapi1.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 105 CLOSED 0 flapi1.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 102 CLOSED 0 optimized-by.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 109 CLOSED 0 flapi2.rubiconproject.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 110 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 111 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 112 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 113 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 88 CLOSED 6 173.222.116.20:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 114 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 111 CLOSED 0 i2.cdn.turner.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 115 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 112 CLOSED 0 i2.cdn.turner.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 116 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 97 CLOSED 0 i2.cdn.turner.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 117 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 117 CLOSED 0 i2.cdn.turner.com:80 REASON=DONE
[10-18 13:13:43] Torbutton INFO: controlPort >> 650 STREAM 118 NEW 0 i2.cdn.turner.com:80 SOURCE_ADDR=/home/thomas/Arbeit/Tor/debugging/20 111/tor-brows PURPOSE=USER

Those woff resources seem to be interesting in particular as they are put on the catch all circuit by Torbutton, probably because the windows is gone. Not sure whether network requests are still ongoing after that but I somehow doubt it.

This only happens when using unix domain sockets for the socks port.

comment:23 Changed 3 years ago by mcs

The hang during shutdown seems to be a Firefox bug (Kathy and I were able to reproduce it on OSX too). I just created https://bugzilla.mozilla.org/show_bug.cgi?id=1311044

Does the browser shutdown correct when it is idle?

comment:24 in reply to:  23 Changed 3 years ago by gk

Replying to mcs:

The hang during shutdown seems to be a Firefox bug (Kathy and I were able to reproduce it on OSX too). I just created https://bugzilla.mozilla.org/show_bug.cgi?id=1311044

Thanks.

Does the browser shutdown correct when it is idle?

Yes, seems so.

comment:25 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed
Status: needs_reviewneeds_revision

The Torbutton patch looks good to me (although I am not happy either with having a copy of _strUnescape() there as well; I thought a bit about using the one from TorLauncher there, too, but that is probably a bad idea as we want to support the no-TorLauncher case as well). Just two minor nits for the TorLauncher one:

1)

+    // If extensions.torlauncher.socks_ipc_path is empty, a default
+    // default path is used (<tor-data-directory>/socks.socket).

"default" once should be enough :)

2)

+    if (useIPC)
+      TorLauncherLogger.log(3, "ipcFile: " + this.mSOCKSPortInfo.ipcFile.path);
+    else
+    {
+      TorLauncherLogger.log(3, "SOCKS host: " + this.mSOCKSPortInfo.host);
+      TorLauncherLogger.log(3, "SOCKS port: " + this.mSOCKSPortInfo.port);
+    }

Please don't mix code parts with and without curly braces in one if-else construct. This is too error-prone for my taste.

That said I tested the patches again with the proposed fix for bug 1311044 and there is not shutdown hang anymore. However, seeing all the output after tor is supposedly shut down still makes me nervous. I think we should have the behavior we currently have in this regard (at least it seems to me we have it that way at the moment): first no traffic anymore, then tor gets shut down and then the browser goes away.

comment:26 in reply to:  25 ; Changed 3 years ago by mcs

Replying to gk:

The Torbutton patch looks good to me (although I am not happy either with having a copy of _strUnescape() there as well; I thought a bit about using the one from TorLauncher there, too, but that is probably a bad idea as we want to support the no-TorLauncher case as well).

Yes, that is why we did not rely on making a call into Tor Launcher.

Just two minor nits for the TorLauncher one:

1)

+    // If extensions.torlauncher.socks_ipc_path is empty, a default
+    // default path is used (<tor-data-directory>/socks.socket).

"default" once should be enough :)

2)

+    if (useIPC)
+      TorLauncherLogger.log(3, "ipcFile: " + this.mSOCKSPortInfo.ipcFile.path);
+    else
+    {
+      TorLauncherLogger.log(3, "SOCKS host: " + this.mSOCKSPortInfo.host);
+      TorLauncherLogger.log(3, "SOCKS port: " + this.mSOCKSPortInfo.port);
+    }

Please don't mix code parts with and without curly braces in one if-else construct. This is too error-prone for my taste.

Thanks for the review. We will fix both of these issue and post a new patch.

That said I tested the patches again with the proposed fix for bug 1311044 and there is not shutdown hang anymore. However, seeing all the output after tor is supposedly shut down still makes me nervous. I think we should have the behavior we currently have in this regard (at least it seems to me we have it that way at the moment): first no traffic anymore, then tor gets shut down and then the browser goes away.

This is not new behavior. We were able to reproduce it using TB 6.5a3 when running with TCP control and SOCKS ports. I will attach a log.

It surprises me that Firefox does not cancel all network activity as a browser tab is closed, but maybe that hurts performance. I don't think any harm is done in this case because, even though the catch all circuit would be used, there is no tor to talk to any more. But I wonder if this same situation can happen when closing a window or tab without exiting the browser. Probably we should open a new ticket for this issue.

Changed 3 years ago by mcs

Attachment: tb6.5a3-quit-log.txt added

TB 6.5a3 log (quit on OSX while cnn.com is loading)

comment:27 in reply to:  26 ; Changed 3 years ago by bugzilla

Replying to mcs:

It surprises me that Firefox does not cancel all network activity as a browser tab is closed, but maybe that hurts performance. I don't think any harm is done in this case because, even though the catch all circuit would be used, there is no tor to talk to any more. But I wonder if this same situation can happen when closing a window or tab without exiting the browser. Probably we should open a new ticket for this issue.

Seems you didn't see ticket:18047#comment:3. You'll be more surprised to know that webpages continue to stay in a cache, so that it's possible to reopen them in a old state.
(also you've just disclosed your timezone ;)

comment:28 in reply to:  27 Changed 3 years ago by mcs

Replying to bugzilla:

Seems you didn't see ticket:18047#comment:3.

I am sure I saw it but did not realize there were implications for isolation.

You'll be more surprised to know that webpages continue to stay in a cache, so that it's possible to reopen them in a old state.

I am not sure if that is a problem or not. Network traffic may be more of a concern, especially if isolation does not occur.

(also you've just disclosed your timezone ;)

Thanks. That is not a big concern to me, but it might be for some people.

comment:29 in reply to:  26 Changed 3 years ago by gk

Replying to mcs:

Replying to gk:

That said I tested the patches again with the proposed fix for bug 1311044 and there is not shutdown hang anymore. However, seeing all the output after tor is supposedly shut down still makes me nervous. I think we should have the behavior we currently have in this regard (at least it seems to me we have it that way at the moment): first no traffic anymore, then tor gets shut down and then the browser goes away.

This is not new behavior. We were able to reproduce it using TB 6.5a3 when running with TCP control and SOCKS ports. I will attach a log.

It surprises me that Firefox does not cancel all network activity as a browser tab is closed, but maybe that hurts performance. I don't think any harm is done in this case because, even though the catch all circuit would be used, there is no tor to talk to any more. But I wonder if this same situation can happen when closing a window or tab without exiting the browser. Probably we should open a new ticket for this issue.

Woah, that was news to me. I actually am seeing this behavior for the first time. I opened #20411.

comment:30 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: needs_revisionneeds_review

comment:31 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. Applied to torbutton master (commit f4980a6c647ea780eacc922846e81c0d992cbd13) and tor-launcher master (commit 8ca52414916c3d8bc2a2974017d759901ddc1736) and maint-0.2.10 (commit 239131f5747385e832d6d12f30382ce00056591b)

Last edited 3 years ago by gk (previous) (diff)
Note: See TracTickets for help on using tickets.