Fix blocklist load integrity: short reads and premature hosts mtime stamp

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<String, Boolean> mapHostsBlocked;
+        private final Logger logger;
+        private long lastModified;
+
+        State(Map<String, Boolean> mapHostsBlocked, long lastModified) {
+            this(mapHostsBlocked, lastModified, NOOP_LOGGER);
+        }
+
+        State(Map<String, Boolean> 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() {
+    }
+}

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 {

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<String> loadExcludedApps(Context c) {
         Set<String> 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<String> loadExcludedApps(Context c) {
 
     private static Set<String> loadBrowserApps(Context c) {
         Set<String> 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));

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();

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<String, Boolean> 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() {
+        }
+    }
+}