From f54a96cd2717c310937e46efdb6e323d355af58d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Apr 2026 20:45:09 +0000 Subject: [PATCH] Fix lock deadlocks, production NPE, connection leaks, and log typo - Wrap all lock.readLock/writeLock operations in try-finally blocks in ServiceSinkhole to prevent permanent deadlocks if exceptions occur while locks are held (e.g. during DB access in blockKnownTracker). Also fixes prepareHostsBlocked where unlock() ran unconditionally even when the lock was never acquired due to early return. - Replace disabled assert + chained NPE in tracker logcat logging: assert is a no-op in production Android, and ipToHost.get(daddr) can return null, causing a crash when log_logcat is enabled. - Fix copy-paste bug in ActivityMain.onActivityResult logging requestCode instead of resultCode. - Add HttpURLConnection.disconnect() in finally blocks in Common.kt and Util.java to prevent socket leaks on repeated network calls. - Move svg.renderToPicture() off the main thread in CountriesFragment; it was executing inside View.post() which runs on the UI thread. https://claude.ai/code/session_011HzyMPo2gd34cnXG4EWLYj --- .../eu/faircode/netguard/ActivityMain.java | 2 +- .../eu/faircode/netguard/ServiceSinkhole.java | 227 ++++++++++-------- .../main/java/eu/faircode/netguard/Util.java | 5 +- .../java/net/kollnig/missioncontrol/Common.kt | 19 +- .../details/CountriesFragment.java | 2 +- 5 files changed, 139 insertions(+), 116 deletions(-) diff --git a/app/src/main/java/eu/faircode/netguard/ActivityMain.java b/app/src/main/java/eu/faircode/netguard/ActivityMain.java index a04704a0..e4f15bb2 100644 --- a/app/src/main/java/eu/faircode/netguard/ActivityMain.java +++ b/app/src/main/java/eu/faircode/netguard/ActivityMain.java @@ -571,7 +571,7 @@ public void onDestroy() { @Override protected void onActivityResult(int requestCode, int resultCode, final Intent data) { - Log.i(TAG, "onActivityResult request=" + requestCode + " result=" + requestCode + " ok=" + Log.i(TAG, "onActivityResult request=" + requestCode + " result=" + resultCode + " ok=" + (resultCode == RESULT_OK)); Util.logExtras(data); diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 8a0670ad..cdd8401c 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -1683,12 +1683,15 @@ private void startNative(final ParcelFileDescriptor vpn, List listAllowed, prepareForwarding(); } else { lock.writeLock().lock(); - mapUidAllowed.clear(); - mapUidKnown.clear(); - mapHostsBlocked.clear(); - mapUidIPFilters.clear(); - mapForward.clear(); - lock.writeLock().unlock(); + try { + mapUidAllowed.clear(); + mapUidKnown.clear(); + mapHostsBlocked.clear(); + mapUidIPFilters.clear(); + mapForward.clear(); + } finally { + lock.writeLock().unlock(); + } } if (log || log_app || filter) { @@ -1754,26 +1757,30 @@ private void stopNative(ParcelFileDescriptor vpn) { private void unprepare() { lock.writeLock().lock(); - mapUidAllowed.clear(); - mapUidKnown.clear(); - mapHostsBlocked.clear(); - mapUidIPFilters.clear(); - mapForward.clear(); - lock.writeLock().unlock(); + try { + mapUidAllowed.clear(); + mapUidKnown.clear(); + mapHostsBlocked.clear(); + mapUidIPFilters.clear(); + mapForward.clear(); + } finally { + lock.writeLock().unlock(); + } } private void prepareUidAllowed(List listAllowed, List listRule) { lock.writeLock().lock(); + try { + mapUidAllowed.clear(); + for (Rule rule : listAllowed) + mapUidAllowed.put(rule.uid, true); - mapUidAllowed.clear(); - for (Rule rule : listAllowed) - mapUidAllowed.put(rule.uid, true); - - mapUidKnown.clear(); - for (Rule rule : listRule) - mapUidKnown.put(rule.uid, rule.uid); - - lock.writeLock().unlock(); + mapUidKnown.clear(); + for (Rule rule : listRule) + mapUidKnown.put(rule.uid, rule.uid); + } finally { + lock.writeLock().unlock(); + } } public static void prepareHostsBlocked(Context c) { @@ -1799,27 +1806,31 @@ public static void prepareHostsBlocked(Context c) { } lock.writeLock().lock(); - mapHostsBlocked.clear(); + try { + 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++; - mapHostsBlocked.put(words[1], true); - } else - Log.i(TAG, "Invalid hosts file line: " + line); - } - } - mapHostsBlocked.put("test.netguard.me", true); - Log.i(TAG, count + " hosts read"); + 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++; + mapHostsBlocked.put(words[1], true); + } else + Log.i(TAG, "Invalid hosts file line: " + line); + } + } + mapHostsBlocked.put("test.netguard.me", true); + Log.i(TAG, count + " hosts read"); + } finally { + lock.writeLock().unlock(); + } } catch (IOException ex) { Log.e(TAG, ex.toString() + "\n" + Log.getStackTraceString(ex)); } finally { @@ -1837,19 +1848,17 @@ public static void prepareHostsBlocked(Context c) { } } - lock.writeLock().unlock(); - // Reload TrackerList to ensure it stays in sync with updated hosts TrackerList.reloadTrackerData(c); } private void prepareUidIPFilters(String dname) { lock.writeLock().lock(); + try { + if (dname == null) // reset mechanism, called from startNative() + mapUidIPFilters.clear(); - if (dname == null) // reset mechanism, called from startNative() - mapUidIPFilters.clear(); - - try (Cursor cursor = DatabaseHelper.getInstance(ServiceSinkhole.this).getAccessDns(dname)) { + try (Cursor cursor = DatabaseHelper.getInstance(ServiceSinkhole.this).getAccessDns(dname)) { int colUid = cursor.getColumnIndex("uid"); int colVersion = cursor.getColumnIndex("version"); int colProtocol = cursor.getColumnIndex("protocol"); @@ -1908,13 +1917,15 @@ private void prepareUidIPFilters(String dname) { } } } - - lock.writeLock().unlock(); + } finally { + lock.writeLock().unlock(); + } } private void prepareForwarding() { lock.writeLock().lock(); - mapForward.clear(); + try { + mapForward.clear(); SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this); if (prefs.getBoolean("filter", true)) { @@ -1961,7 +1972,9 @@ private void prepareForwarding() { Log.i(TAG, "DoH Forward " + dnsFwd); } } - lock.writeLock().unlock(); + } finally { + lock.writeLock().unlock(); + } } private List getAllowedRules(List listRule) { @@ -2122,68 +2135,69 @@ public static void clearTrackerCaches() { private Allowed isAddressAllowed(Packet packet) { SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this); + Allowed allowed = null; lock.readLock().lock(); + try { + packet.allowed = false; + if (prefs.getBoolean("filter", true)) { + // https://android.googlesource.com/platform/system/core/+/master/include/private/android_filesystem_config.h + if (packet.protocol == 17 /* UDP */ && !prefs.getBoolean("filter_udp", true)) { + // Allow unfiltered UDP + packet.allowed = true; + Log.i(TAG, "Allowing UDP " + packet); + } else if ((packet.uid < 2000) && + !mapUidKnown.containsKey(packet.uid) && isSupported(packet.protocol)) { + // Allow unknown (system) traffic + packet.allowed = true; + Log.w(TAG, "Allowing unknown system " + packet); + } else if (packet.uid == Process.myUid()) { + // Allow self + packet.allowed = true; + Log.w(TAG, "Allowing self " + packet); + } else { + boolean filtered = false; - packet.allowed = false; - if (prefs.getBoolean("filter", true)) { - // https://android.googlesource.com/platform/system/core/+/master/include/private/android_filesystem_config.h - if (packet.protocol == 17 /* UDP */ && !prefs.getBoolean("filter_udp", true)) { - // Allow unfiltered UDP - packet.allowed = true; - Log.i(TAG, "Allowing UDP " + packet); - } else if ((packet.uid < 2000) && - !mapUidKnown.containsKey(packet.uid) && isSupported(packet.protocol)) { - // Allow unknown (system) traffic - packet.allowed = true; - Log.w(TAG, "Allowing unknown system " + packet); - } else if (packet.uid == Process.myUid()) { - // Allow self - packet.allowed = true; - Log.w(TAG, "Allowing self " + packet); - } else { - boolean filtered = false; + if (packet.data != null && !packet.data.isEmpty()) + Log.d(TAG, "Found SNI in isAddressAllowed: " + packet.data); - if (packet.data != null && !packet.data.isEmpty()) - Log.d(TAG, "Found SNI in isAddressAllowed: " + packet.data); + // Check if tracker is known + // In minimal mode (including TC Slim), always enable blocking + if (blockKnownTracker(packet.daddr, packet.uid)) { + filtered = true; + packet.allowed = false; + } - // Check if tracker is known - // In minimal mode (including TC Slim), always enable blocking - if (blockKnownTracker(packet.daddr, packet.uid)) { - filtered = true; - packet.allowed = false; - } + InternetBlocklist internetBlocklist = InternetBlocklist.getInstance(ServiceSinkhole.this); + if (internetBlocklist.blockedInternet(packet.uid)) { + filtered = true; + packet.allowed = false; + } - InternetBlocklist internetBlocklist = InternetBlocklist.getInstance(ServiceSinkhole.this); - if (internetBlocklist.blockedInternet(packet.uid)) { - filtered = true; - packet.allowed = false; + if (!filtered) + packet.allowed = true; } - - if (!filtered) - packet.allowed = true; } - } - // Block DNS-over-TLS (DoT) on port 853 to prevent bypassing DNS filtering - if (packet.allowed && packet.dport == 853 && prefs.getBoolean("block_dot", true)) { - Log.i(TAG, "Blocking DoT " + packet); - packet.allowed = false; - } + // Block DNS-over-TLS (DoT) on port 853 to prevent bypassing DNS filtering + if (packet.allowed && packet.dport == 853 && prefs.getBoolean("block_dot", true)) { + Log.i(TAG, "Blocking DoT " + packet); + packet.allowed = false; + } - Allowed allowed = null; - if (packet.allowed) - if (mapForward.containsKey(packet.dport)) { - Forward fwd = mapForward.get(packet.dport); - if (fwd.ruid == packet.uid) { + if (packet.allowed) + if (mapForward.containsKey(packet.dport)) { + Forward fwd = mapForward.get(packet.dport); + if (fwd.ruid == packet.uid) { + allowed = new Allowed(); + } else { + allowed = new Allowed(fwd.raddr, fwd.rport); + packet.data = "> " + fwd.raddr + "/" + fwd.rport; + } + } else allowed = new Allowed(); - } else { - allowed = new Allowed(fwd.raddr, fwd.rport); - packet.data = "> " + fwd.raddr + "/" + fwd.rport; - } - } else - allowed = new Allowed(); - - lock.readLock().unlock(); + } finally { + lock.readLock().unlock(); + } if (prefs.getBoolean("log", false) || prefs.getBoolean("log_app", true)) if (packet.protocol != 6 /* TCP */ || !"".equals(packet.flags)) @@ -2347,8 +2361,11 @@ private boolean blockKnownTracker(String daddr, int uid) { app = Common.getAppName(pm, uid); uidToApp.put(uid, app); } - assert tracker != null; - Log.i("TC-Log", app + " " + daddr + " " + ipToHost.get(daddr).getOrExpired() + " " + tracker.getName()); + if (tracker != null) { + Expiring host = ipToHost.get(daddr); + String hostName = (host != null) ? host.getOrExpired() : null; + Log.i("TC-Log", app + " " + daddr + " " + hostName + " " + tracker.getName()); + } } else { if (tracker != NO_TRACKER) { boolean blockedByGranularRule = false; diff --git a/app/src/main/java/eu/faircode/netguard/Util.java b/app/src/main/java/eu/faircode/netguard/Util.java index 4a89443f..283c9686 100644 --- a/app/src/main/java/eu/faircode/netguard/Util.java +++ b/app/src/main/java/eu/faircode/netguard/Util.java @@ -586,9 +586,10 @@ public static String getOrganization(String ip) throws Exception { return mapIPOrganization.get(ip); } BufferedReader reader = null; + HttpURLConnection connection = null; try { URL url = new URL("https://ipinfo.io/" + ip + "/org"); - HttpURLConnection connection = (HttpURLConnection) url.openConnection(); + connection = (HttpURLConnection) url.openConnection(); connection.setRequestProperty("Accept-Encoding", "gzip"); connection.setRequestMethod("GET"); connection.setReadTimeout(15 * 1000); @@ -607,6 +608,8 @@ public static String getOrganization(String ip) throws Exception { } finally { if (reader != null) reader.close(); + if (connection != null) + connection.disconnect(); } } diff --git a/app/src/main/java/net/kollnig/missioncontrol/Common.kt b/app/src/main/java/net/kollnig/missioncontrol/Common.kt index 553b0b31..46dd4d11 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/Common.kt +++ b/app/src/main/java/net/kollnig/missioncontrol/Common.kt @@ -43,27 +43,30 @@ object Common { */ @JvmStatic fun fetch(url: String?): String? { + var conn: HttpURLConnection? = null try { val html = StringBuilder() - val conn = (URL(url)).openConnection() as HttpURLConnection + conn = (URL(url)).openConnection() as HttpURLConnection conn.setRequestProperty("Accept-Encoding", "gzip") conn.setConnectTimeout(5000) - val `in`: BufferedReader? - if ("gzip" == conn.getContentEncoding()) `in` = + val reader: BufferedReader = if ("gzip" == conn.getContentEncoding()) BufferedReader(InputStreamReader(GZIPInputStream(conn.getInputStream()))) - else `in` = BufferedReader(InputStreamReader(conn.getInputStream())) + else + BufferedReader(InputStreamReader(conn.getInputStream())) - var str: String? - while ((`in`.readLine().also { str = it }) != null) html.append(str) - - `in`.close() + reader.use { r -> + var str: String? + while ((r.readLine().also { str = it }) != null) html.append(str) + } return html.toString() } catch (e: IOException) { return null } catch (e: OutOfMemoryError) { return null + } finally { + conn?.disconnect() } } diff --git a/app/src/main/java/net/kollnig/missioncontrol/details/CountriesFragment.java b/app/src/main/java/net/kollnig/missioncontrol/details/CountriesFragment.java index 537d7318..957bd0f9 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/details/CountriesFragment.java +++ b/app/src/main/java/net/kollnig/missioncontrol/details/CountriesFragment.java @@ -163,8 +163,8 @@ private void showCountriesMap(ProgressBar pbLoading, ImageView mv, TextView txtF String countries = TextUtils.join(",#", hostCountriesCount.keySet()); renderOptions.css(String.format("#%s { fill: #B71C1C; }", countries.toUpperCase())); + Picture picture = svg.renderToPicture(renderOptions); mv.post(() -> { - Picture picture = svg.renderToPicture(renderOptions); mv.setImageDrawable(new PictureDrawable(picture)); pbLoading.setVisibility(View.GONE); });