Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1370)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by Felix Dahlke
Modified:
4 years, 6 months ago
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 ...
4 years, 6 months ago (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 ...
4 years, 6 months ago (2015-01-13 14:12:33 UTC) #2
Felix Dahlke
4 years, 6 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5