Fix blocklist load integrity: short reads and premature hosts mtime stamp - #773
Merged
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two confirmed silent-degradation defects in the Java-side blocklist loading path.
Defect 1 — asset loads assume a single
read(byte[])fills the bufferapp/src/main/java/net/kollnig/missioncontrol/data/TrackerList.java:585-588(loadDisconnectTrackers)app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java:168-172(loadExcludedApps)app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java:187-191(loadBrowserApps)Each sizes a buffer from
is.available()and then callsis.read(buffer)once, treating any positive return as a full read.InputStream.read(byte[])may legally return fewer bytes. These assets are deflate-compressed — there is nonoCompressentry for them inapp/build.gradle— so the contract violation is real; it does not bite today only because Android'sStreamingZipInflaterhappens to loop internally. A short read would feed truncated bytes straight into JSON parsing: a partial Disconnect list (detection quietly degrades) or empty minimal-mode exclusions / browser classification, with only a log line.Fixed by wrapping the asset stream in
DataInputStreamand usingreadFully().EOFExceptionis anIOException, so the existing catch/log behaviour is unchanged.Defect 2 — hosts-file mtime stamped before the parse succeeds
app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java:2151(prepareHostsBlocked) setlast_hosts_modified = hosts.lastModified()before the parse loop, and anIOExceptionmid-parse is swallowed by thecatchat:2179. The early return at:2146-2149(!changed && mapHostsBlocked.size() > 0) then short-circuits every later call.Accurate impact — narrower than issue #762 states: the other early return at
:2140is guarded bymapHostsBlocked.size() > 0, so a total failure (map ends up empty) does still retry. Only a partial parse gets pinned: the map keeps whatever prefix was read until the file's mtime changes again.Fixed by reading the mtime once before opening the reader and committing it to
last_hosts_modifiedonly after the parse loop completes without throwing. On failure the field keeps its previous value, so the next call re-reads.The parse loop and the reload/commit state machine move into a small pure helper,
eu.faircode.netguard.HostsBlocklistLogic, following the existingNetworkReloadPolicy/VpnRevokePolicy/BlockingModeLogicpattern in this repo, so the retry behaviour is unit-testable. Parsing behaviour is byte-for-byte identical:#comment stripping, 2-word line validation,Locale.ROOTlowercase keying, thetest.netguard.meentry, and the same log lines.Deliberately excluded
Issue #762 parts (b) (a deleted
hosts.txtkeeps stale entries) and (c) (a missing file silently restores the bundled ~90k-domain StevenBlack asset) are not addressed here. Both change fresh-install and post-deletion defaults, which is a product decision that has not been made — whether a missing file should be treated as authoritative-empty, and whether the bundled fallback should be surfaced to the user. They need their own PR once that call is made.Test evidence
New
app/src/test/java/eu/faircode/netguard/HostsBlocklistLogicTest.javadrives aReaderthat throwsIOExceptionafter the first line and asserts:shouldReload(sameMtime)is still true — the partial map is not pinned;Assertion (1) fails against the pre-fix ordering.
TEST-eu.faircode.netguard.HostsBlocklistLogicTest.xml:tests="1" skipped="0" failures="0" errors="0".No short-read regression test: the three call sites read directly from
AssetManagerwith no injectable seam, and adding one purely for this would mean widening the API surface for a test.Merge notes
ServiceSinkhole.javais also touched by open PRs #752 and #768, ingetBuilder(:1762),:945) andLogHandler(onCreate/onDestroy(:3209/:3691). This change stays inside:2132-2170), plus dropping the now-unusedprepareHostsBlocked(java.util.Localeimport.Fixes #758
Partially addresses #762 (part (a) only).