From 290b60733efe0eecf25c6b3adef68d6eeee672e1 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:18:53 +0200 Subject: [PATCH 1/2] Fix DNS cache invalidation race in tracker attribution (#757) blockKnownTracker() reads DNS evidence from DatabaseHelper under its read lock, then writes ipToHost/ipToTracker outside any lock. A packet thread that read the DB before a concurrent dnsResolved() insert can still write its now-stale verdict after dnsResolved() has cleared the cache entry for that IP, pinning pre-insert attribution. Fix with a generation counter: dnsResolved() bumps it after clearing the cache (inside the same insertDns()-succeeded, numeric-address block as the removes), and blockKnownTracker() snapshots it before the DB read and skips both puts if it changed during the read. Skipping is free: the next packet to that IP just re-reads the DB. A single global counter can cause the skip on any concurrent DNS answer, not just ones for the same IP, but the window per answer is a DB read (microseconds) against a DNS answer rate that is low even on noisy apps, so the false-skip rate is negligible and costs one extra DB read on the rare hit. A per-IP scheme would avoid that but adds new unbounded state (a map that must itself be cleaned up), which is worse than the cost it removes. Not adding a unit test: the guard is a two-line long comparison with no independent behaviour to verify; the actual race lives in the interleaving of two threads through DatabaseHelper's real lock, which would need a much larger extraction than this fix to test in isolation. Co-Authored-By: Claude Opus 5 --- .../eu/faircode/netguard/ServiceSinkhole.java | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index b6b7c723..026786cb 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -125,6 +125,7 @@ import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.zip.GZIPInputStream; @@ -2457,6 +2458,13 @@ private void dnsResolved(ResourceRecord rr) { if (Util.isNumericAddress(rr.Resource)) { // make sure correct format ipToHost.remove(rr.Resource); ipToTracker.remove(rr.Resource); + // Bump *after* the removes: a blockKnownTracker() read that + // started before this insert (and so may have missed this row) + // can still be mid-flight. Invalidating the generation here, + // after the cache is actually clear, is what makes its stale + // put() get discarded below instead of pinning pre-insert + // attribution behind this remove. + trackerCacheGeneration.incrementAndGet(); } } } @@ -2507,6 +2515,11 @@ private boolean isSupported(int protocol) { private static final ConcurrentHashMap uidToPackage = new ConcurrentHashMap<>(); static ConcurrentHashMap> ipToHost = new ConcurrentHashMap<>(); static ConcurrentHashMap> ipToTracker = new ConcurrentHashMap<>(); + // Bumped by dnsResolved() whenever it invalidates an ipToHost/ipToTracker + // entry, so blockKnownTracker() can detect a DB read that raced a + // concurrent insert and drop its (possibly stale) result instead of + // caching it. See the comments at both call sites. + private static final AtomicLong trackerCacheGeneration = new AtomicLong(); static String NO_DNAME = "null"; // use a String, unequal the real null static Tracker NO_TRACKER = new Tracker(null, null, 0); // Negative results (no tracker / no dname for an IP) are cached only @@ -2684,6 +2697,13 @@ private boolean blockKnownTracker(String daddr, int uid) { } if (dname == null) { // TODO: Note that this does not implement any SNI code + // Snapshot before the DB read: if dnsResolved() invalidates this + // IP's cache entry while we're mid-read below, our result may be + // stale (it could miss a row that raced us, or reflect a row + // that no longer applies). Comparing after the read lets us + // drop the put and let the next packet re-read instead of + // pinning a possibly-wrong verdict. + long generationBefore = trackerCacheGeneration.get(); // Retrieve dname from DB DatabaseHelper dh = DatabaseHelper.getInstance(ServiceSinkhole.this); long now = new Date().getTime(); @@ -2781,9 +2801,16 @@ private boolean blockKnownTracker(String daddr, int uid) { : firstTime + firstTtl; } - // Save dname and tracker - ipToHost.put(daddr, new Expiring<>(dname, expiry)); - ipToTracker.put(daddr, new Expiring<>(tracker, expiry)); + // Save dname and tracker, but only if no concurrent + // dnsResolved() invalidated this IP's cache while we were + // reading the DB above — otherwise this put could pin a + // stale verdict that a racing insert already made obsolete. + // Skipping is cheap and correct: the next packet to this IP + // simply re-reads the DB. + if (trackerCacheGeneration.get() == generationBefore) { + ipToHost.put(daddr, new Expiring<>(dname, expiry)); + ipToTracker.put(daddr, new Expiring<>(tracker, expiry)); + } } // Do not block based on IP-only tracker evidence. From 8d2c68ea6fe8f8cc4e6975b134eb4284365dc60b Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:22:17 +0200 Subject: [PATCH 2/2] Bump generation counter at the other two cache-invalidation sites too clearTrackerCaches() (called from BlockingMode.applyMode() on every blocking-mode change) and householding()'s 12-hourly wholesale clear both wipe ipToHost/ipToTracker without bumping trackerCacheGeneration, leaving the same unlocked-put race dnsResolved() was fixed for: an in-flight blockKnownTracker() put can still land right after either clear and re-pin a stale entry. clearTrackerCaches() is the one that matters: blockKnownTracker() reads blockAmbiguousTrackers from the current blocking mode at the top of the method, before any cache or DB read, and uses it to resolve mixed tracker/non-tracker DNS evidence. If the mode changes mid-call, the clear wipes the caches but an in-flight put can still pin a verdict computed under the old mode. householding() has the same gap but much lower impact, since the next 12h cycle clears it again regardless. Co-Authored-By: Claude Opus 5 --- app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 026786cb..02949d14 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -829,6 +829,8 @@ private void householding(Intent intent) { // Refresh mappings regularly ipToHost.clear(); ipToTracker.clear(); + // Same race as dnsResolved(): invalidate after clearing. + trackerCacheGeneration.incrementAndGet(); uidToApp.clear(); uidToPackage.clear(); @@ -2530,6 +2532,9 @@ private boolean isSupported(int protocol) { public static void clearTrackerCaches() { ipToHost.clear(); ipToTracker.clear(); + // Same race as dnsResolved(): invalidate after clearing, so a + // blockKnownTracker() put in flight under the old mode is dropped. + trackerCacheGeneration.incrementAndGet(); } // Called from native code