diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index b6b7c723..02949d14 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; @@ -828,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(); @@ -2457,6 +2460,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 +2517,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 @@ -2517,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 @@ -2684,6 +2702,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 +2806,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.