#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
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs