Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6006698351263744: Issue 1498 - Look for iptables on the system first (adblockplusandroid 1.2.1) (Closed)

Created:
Jan. 9, 2015, 6:46 p.m. by Felix Dahlke
Modified:
Jan. 14, 2015, 6:21 p.m.
Visibility:
Public.

Description

Issue 1498 - Look for iptables on the system first Lollipop needs iptables as PIE executable, the one we ship doesn't fit the bill. We could embed two different iptables executables, but I'd rather avoid that if possible. Increasing the minimum SDK version to 16, however, would be worse. Fortunately, we can apparently just use the iptables executable in /system/bin, at least on stock Lollipop. (Please note that this patch is for adblockplusandroid 1.2.1, there'll be a separate one based on the tip revision.)

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -1 line) Patch
M src/org/adblockplus/android/ProxyService.java View 1 chunk +6 lines, -1 line 2 comments Download

Messages

Total messages: 3
Felix Dahlke
http://codereview.adblockplus.org/6006698351263744/diff/5629499534213120/src/org/adblockplus/android/ProxyService.java File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/6006698351263744/diff/5629499534213120/src/org/adblockplus/android/ProxyService.java#newcode586 src/org/adblockplus/android/ProxyService.java:586: File ipt = new File("/system/bin/iptables"); It _seems_ like a ...
Jan. 9, 2015, 6:51 p.m. (2015-01-09 18:51:58 UTC) #1
Wladimir Palant
LGTM but I have doubts that this fixes the entire issue. Are we certain that ...
Jan. 13, 2015, 2:12 p.m. (2015-01-13 14:12:33 UTC) #2
Felix Dahlke
Jan. 13, 2015, 3:01 p.m. (2015-01-13 15:01:44 UTC) #3
For the record, we discussed this on IRC a bit:

- Nexi running stock Android 5.x all seem to have /system/bin/iptables
- Android has been using /system/bin/iptables internally since around Honeycomb:
https://android.googlesource.com/platform/system/netd/+/4a5f5ca3c9e07fc3e6fec...

Seems like the vast majority of our users should have that binary.

On 2015/01/13 14:12:33, Wladimir Palant wrote:
> LGTM but I have doubts that this fixes the entire issue. Are we certain that
> iptables is always present on newer Android versions? Can we use our test
> infrastructure to verify that?
> 
>
http://codereview.adblockplus.org/6006698351263744/diff/5629499534213120/src/...
> File src/org/adblockplus/android/ProxyService.java (right):
> 
>
http://codereview.adblockplus.org/6006698351263744/diff/5629499534213120/src/...
> src/org/adblockplus/android/ProxyService.java:586: File ipt = new
> File("/system/bin/iptables");
> On 2015/01/09 18:51:59, Felix H. Dahlke wrote:
> > It _seems_ like a good idea to always go for iptables on the system, if it
> > exists.
> 
> The concern was mostly that an unknown iptables version might have different
> parameters. However, that doesn't seem to be very likely.
> 
> > Do older Android
> > versions also have an iptables executable, one we couldn't use?
> 
> The question is rather: do newer Android versions? Back when this code was
> introduced, iptables was occasionally installed by the hardware vendor or
> rooting toolkit - but it wasn't part of the regular Android setup.

Powered by Google App Engine
This is Rietveld