From f42ffe0cd0d12afc9b1c31ce6207fc68a0e7f09f Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 12:07:59 +0200 Subject: [PATCH] Fix blocklist load integrity: short reads and premature mtime stamp Two silent-degradation defects in the Java-side blocklist loading path. 1. Blocklist asset loads assumed a single read(byte[]) filled the buffer (TrackerList.loadDisconnectTrackers, BlockingMode.loadExcludedApps, BlockingMode.loadBrowserApps). The assets are deflate-compressed, so the contract violation is real. Use DataInputStream.readFully(). 2. ServiceSinkhole.prepareHostsBlocked stamped last_hosts_modified before the parse loop ran, so an IOException mid-parse left a partially populated map pinned behind the "Hosts file unchanged" early return. The mtime is now committed only after a fully successful parse. The hosts parse plus the reload/commit state machine move into a small pure helper (HostsBlocklistLogic) so the retry behaviour can be unit tested; behaviour is otherwise unchanged. Fixes #758 Partially addresses #762 (part a only) Co-Authored-By: Claude Opus 5 --- .../netguard/HostsBlocklistLogic.java | 78 +++++++++++++++++++ .../eu/faircode/netguard/ServiceSinkhole.java | 39 ++++------ .../missioncontrol/data/BlockingMode.java | 14 ++-- .../missioncontrol/data/TrackerList.java | 7 +- .../netguard/HostsBlocklistLogicTest.java | 70 +++++++++++++++++ 5 files changed, 175 insertions(+), 33 deletions(-) create mode 100644 app/src/main/java/eu/faircode/netguard/HostsBlocklistLogic.java create mode 100644 app/src/test/java/eu/faircode/netguard/HostsBlocklistLogicTest.java diff --git a/app/src/main/java/eu/faircode/netguard/HostsBlocklistLogic.java b/app/src/main/java/eu/faircode/netguard/HostsBlocklistLogic.java new file mode 100644 index 000000000..28c1123bc --- /dev/null +++ b/app/src/main/java/eu/faircode/netguard/HostsBlocklistLogic.java @@ -0,0 +1,78 @@ +package eu.faircode.netguard; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.Reader; +import java.util.Locale; +import java.util.Map; + +final class HostsBlocklistLogic { + interface Logger { + void log(String message); + } + + private static final Logger NOOP_LOGGER = message -> { + }; + + static final class State { + private final Map mapHostsBlocked; + private final Logger logger; + private long lastModified; + + State(Map mapHostsBlocked, long lastModified) { + this(mapHostsBlocked, lastModified, NOOP_LOGGER); + } + + State(Map mapHostsBlocked, long lastModified, Logger logger) { + this.mapHostsBlocked = mapHostsBlocked; + this.lastModified = lastModified; + this.logger = logger; + } + + boolean shouldReload(long modified) { + return modified != lastModified || mapHostsBlocked.size() == 0; + } + + boolean load(Reader reader, long modified) throws IOException { + if (!shouldReload(modified)) + return false; + + parse(reader); + lastModified = modified; + return true; + } + + void parse(Reader reader) throws IOException { + mapHostsBlocked.clear(); + BufferedReader br = reader instanceof BufferedReader + ? (BufferedReader) reader : new BufferedReader(reader); + int count = 0; + String line; + while ((line = br.readLine()) != null) { + int hash = line.indexOf('#'); + if (hash >= 0) + line = line.substring(0, hash); + line = line.trim(); + if (line.length() > 0) { + String[] words = line.split("\\s+"); + if (words.length == 2) { + count++; + // Keyed lowercase to match TrackerList.findTracker(), + // which normalises qnames before the hosts lookup. + mapHostsBlocked.put(words[1].toLowerCase(Locale.ROOT), true); + } else + logger.log("Invalid hosts file line: " + line); + } + } + mapHostsBlocked.put("test.netguard.me", true); + logger.log(count + " hosts read"); + } + + long getLastModified() { + return lastModified; + } + } + + private HostsBlocklistLogic() { + } +} diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 3d330fa24..3e9449adf 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -114,7 +114,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -2134,49 +2133,37 @@ public static void prepareHostsBlocked(Context c) { InputStreamReader is = null; boolean locked = false; File hosts = new File(c.getFilesDir(), "hosts.txt"); + boolean hostsFile = false; + long hostsModified = 0; + HostsBlocklistLogic.State hostsState = new HostsBlocklistLogic.State( + mapHostsBlocked, last_hosts_modified, message -> Log.i(TAG, message)); try { - if (!hosts.exists() || !hosts.canRead()) { + hostsFile = hosts.exists() && hosts.canRead(); + if (!hostsFile) { if (mapHostsBlocked.size() > 0) { Log.i(TAG, "Hosts file unchanged"); return; } is = new InputStreamReader(c.getAssets().open("hosts.txt")); } else { - boolean changed = (hosts.lastModified() != last_hosts_modified); - if (!changed && mapHostsBlocked.size() > 0) { + hostsModified = hosts.lastModified(); + if (!hostsState.shouldReload(hostsModified)) { Log.i(TAG, "Hosts file unchanged"); return; } - last_hosts_modified = hosts.lastModified(); is = new FileReader(hosts); } lock.writeLock().lock(); locked = true; - mapHostsBlocked.clear(); - int count = 0; br = new BufferedReader(is); - String line; - while ((line = br.readLine()) != null) { - int hash = line.indexOf('#'); - if (hash >= 0) - line = line.substring(0, hash); - line = line.trim(); - if (line.length() > 0) { - String[] words = line.split("\\s+"); - if (words.length == 2) { - count++; - // Keyed lowercase to match TrackerList.findTracker(), - // which normalises qnames before the hosts lookup. - mapHostsBlocked.put(words[1].toLowerCase(Locale.ROOT), true); - } else - Log.i(TAG, "Invalid hosts file line: " + line); - } - } - mapHostsBlocked.put("test.netguard.me", true); - Log.i(TAG, count + " hosts read"); + if (hostsFile) { + hostsState.load(br, hostsModified); + last_hosts_modified = hostsState.getLastModified(); + } else + hostsState.parse(br); } catch (IOException ex) { Log.e(TAG, ex.toString() + "\n" + Log.getStackTraceString(ex)); } finally { diff --git a/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java b/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java index 3aadcfa4f..b69a06aee 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java +++ b/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java @@ -23,8 +23,8 @@ import androidx.preference.PreferenceManager; +import java.io.DataInputStream; import java.io.IOException; -import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Collections; @@ -166,11 +166,13 @@ public static boolean isBrowserApp(Context c, String packageName) { private static Set loadExcludedApps(Context c) { Set apps = new HashSet<>(); - try (InputStream is = c.getAssets().open("ddg-excluded-apps.json")) { + try (DataInputStream is = new DataInputStream( + c.getAssets().open("ddg-excluded-apps.json"))) { int size = is.available(); byte[] buffer = new byte[size]; - if (is.read(buffer) <= 0) + if (size <= 0) throw new IOException("No bytes read."); + is.readFully(buffer); String json = new String(buffer, StandardCharsets.UTF_8); apps.addAll(BlockingModeLogic.parseExcludedAppsJson(json)); @@ -184,11 +186,13 @@ private static Set loadExcludedApps(Context c) { private static Set loadBrowserApps(Context c) { Set apps = new HashSet<>(); - try (InputStream is = c.getAssets().open("ddg-excluded-apps.json")) { + try (DataInputStream is = new DataInputStream( + c.getAssets().open("ddg-excluded-apps.json"))) { int size = is.available(); byte[] buffer = new byte[size]; - if (is.read(buffer) <= 0) + if (size <= 0) throw new IOException("No bytes read."); + is.readFully(buffer); String json = new String(buffer, StandardCharsets.UTF_8); apps.addAll(BlockingModeLogic.parseBrowserAppsJson(json)); diff --git a/app/src/main/java/net/kollnig/missioncontrol/data/TrackerList.java b/app/src/main/java/net/kollnig/missioncontrol/data/TrackerList.java index c3077c684..79c8160ab 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/data/TrackerList.java +++ b/app/src/main/java/net/kollnig/missioncontrol/data/TrackerList.java @@ -32,6 +32,7 @@ import eu.faircode.netguard.DatabaseHelper; +import java.io.DataInputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -582,11 +583,13 @@ private void loadDisconnectTrackers(Context c) { * More here: * https://github.com/TrackerControl/tracker-control-android/issues/30 */ - try (InputStream is = c.getAssets().open("disconnect-blacklist.reversed.json")) { + try (DataInputStream is = new DataInputStream( + c.getAssets().open("disconnect-blacklist.reversed.json"))) { int size = is.available(); byte[] buffer = new byte[size]; - if (is.read(buffer) <= 0) + if (size <= 0) throw new IOException("No bytes read."); + is.readFully(buffer); String reversedJson = new String(buffer, StandardCharsets.UTF_8); String json = new StringBuilder(reversedJson).reverse().toString(); diff --git a/app/src/test/java/eu/faircode/netguard/HostsBlocklistLogicTest.java b/app/src/test/java/eu/faircode/netguard/HostsBlocklistLogicTest.java new file mode 100644 index 000000000..94b4d9cf2 --- /dev/null +++ b/app/src/test/java/eu/faircode/netguard/HostsBlocklistLogicTest.java @@ -0,0 +1,70 @@ +package eu.faircode.netguard; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.junit.Test; + +import java.io.IOException; +import java.io.Reader; +import java.io.StringReader; +import java.util.HashMap; +import java.util.Map; + +public class HostsBlocklistLogicTest { + @Test + public void failedParseDoesNotPinPartialMapAtNewMtime() throws Exception { + Map hosts = new HashMap<>(); + HostsBlocklistLogic.State state = new HostsBlocklistLogic.State(hosts, 10L); + + try { + state.load(new FailingReader(), 20L); + fail("Expected the parse to fail"); + } catch (IOException expected) { + // The partial entry remains, matching the service's failure behavior. + } + + assertEquals(10L, state.getLastModified()); + assertTrue(state.shouldReload(20L)); + assertTrue(hosts.containsKey("first.example")); + + assertTrue(state.load(new StringReader( + "1.1.1.1 first.example\n2.2.2.2 second.example\n"), 20L)); + assertEquals(20L, state.getLastModified()); + assertEquals(3, hosts.size()); + assertTrue(hosts.containsKey("first.example")); + assertTrue(hosts.containsKey("second.example")); + assertTrue(hosts.containsKey("test.netguard.me")); + + assertFalse(state.load(new FailingReader(), 20L)); + assertEquals(20L, state.getLastModified()); + assertEquals(3, hosts.size()); + } + + private static final class FailingReader extends Reader { + private final String firstLine = "1.1.1.1 first.example\n"; + private int position; + private boolean failed; + + @Override + public int read(char[] cbuf, int off, int len) throws IOException { + if (failed) + throw new IOException("mid-parse"); + if (position == firstLine.length()) { + failed = true; + throw new IOException("mid-parse"); + } + + int count = Math.min(len, firstLine.length() - position); + firstLine.getChars(position, position + count, cbuf, off); + position += count; + return count; + } + + @Override + public void close() { + } + } +}