[tor-bugs] #30199 [Applications/Tor Browser]: tor-android-service: Review 2019/04/16

[Tor Bug Tracker & Wiki]

#30199: tor-android-service: Review 2019/04/16
-------------------------------------------------+-------------------------
 Reporter:  sysrqb                               |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, tbb-8.5,                 |  Actual Points:
  TorBrowserTeam201909R                          |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by sysrqb):

 * status:  needs_review => needs_revision


Comment:

 Okay, I decided branch `0801a` is probably the correct one.

 `a70452c592ed4b2586a020a234d5835cfb83d704`: seems fine, looks like a
 cherry-pick from Orbot
 `39d7fc1c0e75a56bddfc1c0cc5902b100244fce8`: seems fine, but I don't see
 any use of the new binary tor version pref
 `1513f8d2ccaf99fa9d7e8fe1ae00b1f535697030`:
 1. we'll need to carry a patch for s/Orbot/Tor Browser/.
 1. Were those localizations directly copied from Orbot?
 `03dc6cd426eba5ce43d800356e92a5e89cb65161`: TODO (need to come back for
 this one, very large)
 `4471410b742ddf771b401bfb8b1d2c10a4e884e3`: Okay. (I'd prefer we build
 this ourselves, but really we can delete the entire module.)
 `ac0a066c9ee75c0e869f6c95401b8134a2248c25`:
 {{{
      public String getProxySocks5Host() {
 -        return OrbotVpnManager.sSocksProxyLocalhost;
 +        return "127.0.0.1";
      }
 }}}
 3. We'll want this configurable in the future. See #29498

 In
 `vpn/src/main/java/org/torproject/android/service/vpn/OrbotVpnManager.java`
 {{{
 -    public static int sSocksProxyServerPort = -1;
 -    public static String sSocksProxyLocalhost = null;
 +    private int socksProxyServerPort = -1;
 }}}

 4. Can you use `mSocksProxyServerPort` instead of `socksProxyServerPort`?

 {{{
 -
 -                               sSocksProxyLocalhost = "127.0.0.1";//
 InetAddress.getLocalHost().getHostAddress();
 -                               sSocksProxyServerPort =
 (int)((Math.random()*1000)+10000);
 -
 +
 +                               socksProxyServerPort =
 (int)((Math.random()*1000)+10000);
 +
 prefs.edit().putInt(SOCKS_PROXY_PORT, socksProxyServerPort).apply();
                                         } catch (Exception e) {
                                                 Log.e(TAG,"Unable to
 access localhost",e);
                                                 throw new
 RuntimeException("Unable to access localhost: " + e);
 }}}

 5. The `Log.e` error message is incorrect now.
 6. Is the try-catch necessary now?

 `4c964a06bf1259fbbb0f6972834a8a47b90139ab`: TODO (but it's only changing
 the vpn, so I'm less concerned about it right now)
 `8ece3cb1cc7dbf61ad867d65b7cc5b4fef4522a3`
 `509ef7245d22f8dd5114aa0945632652a370a51e`
 `c7b8d3c8ba1bd2ff10a6a39b32a2501f35c17bcb`
 `4dfca8dfb48797450a52c1cba0a0be2833823b61`
 `7afc999ee941ece9a72356dbb60c3c48bb36c780`
 `5553d81c02720c92ace0982317c5b455425dee14`
 `62e3ac83b7357c9c513b5fa5027817d9e9cf0909`
 `e3f80d70fd6c8e2b8ea0152c861241121e210f05`: Is there a reason you didn't
 cherry-pick these and keep their commit messages?

 ------

 Regarding previous comments,
 `a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304`: We shouldn't modify the
 commit, so I think we can live with the current situation.

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