Synchronize TrackerBlocklist access across packet and UI threads - #750
Merged
Conversation
Readers (blocked/blockedTracker/getSubset) ran unsynchronized on native JNI packet threads while writers on the UI thread mutated the same plain HashSet values, risking ConcurrentModificationException inside the VPN packet path and torn reads. The lazy singleton in getInstance could also split-brain into two instances under concurrent first calls, leaving an orphan instance serving stale decisions. Make all entry points mutually exclusive; no behavior change.
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
1 participant
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.
Problem
TrackerBlocklist's read path (blocked/blockedTracker/getSubset) ran unsynchronized on native JNI packet threads while writers on the UI thread mutated the same plain HashSet values, so concurrent reads could hit ConcurrentModificationException or torn state inside the VPN packet path — where an exception propagates through JNI and can tear down the VPN service. The lazy singleton in getInstance was also not synchronized, so concurrent first calls from packet and UI threads could create two instances, with the orphan serving stale blocking decisions indefinitely. loadSettings additionally rebuilt blockmap.clear()+rebuild while readers could be mid-iteration.
Fix
getInstancea static synchronized methodloadSettings,getSubset,blocked,blockedTracker,clear(),clear(int)synchronized instance methods (joining the already-synchronized writers)Test plan
./gradlew :app:compileGithubDebugJavaWithJavac -q→ exit 0./gradlew :app:testGithubDebugUnitTest -q→ exit 0 (full suite green)