Skip to content

Fix lock deadlocks, production NPE, connection leaks, and log typo #539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kasnder wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix lock deadlocks, production NPE, connection leaks, and log typo #539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/main/java/eu/faircode/netguard/ActivityMain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
227 changes: 122 additions & 105 deletions app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,12 +1683,15 @@ private void startNative(final ParcelFileDescriptor vpn, List<Rule> 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) {
Expand Down Expand Up @@ -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<Rule> listAllowed, List<Rule> 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) {
Expand All @@ -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 {
Expand All @@ -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");
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -1961,7 +1972,9 @@ private void prepareForwarding() {
Log.i(TAG, "DoH Forward " + dnsFwd);
}
}
lock.writeLock().unlock();
} finally {
lock.writeLock().unlock();
}
}

private List<Rule> getAllowedRules(List<Rule> listRule) {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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<String> 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;
Expand Down
5 changes: 4 additions & 1 deletion app/src/main/java/eu/faircode/netguard/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -607,6 +608,8 @@ public static String getOrganization(String ip) throws Exception {
} finally {
if (reader != null)
reader.close();
if (connection != null)
connection.disconnect();
}
}

Expand Down
19 changes: 11 additions & 8 deletions app/src/main/java/net/kollnig/missioncontrol/Common.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/net/kollnig/missioncontrol/details/CountriesFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
Loading