celzero / rethink-app

DNS over HTTPS / DNS over Tor / DNSCrypt client, WireGuard proxifier, firewall, and connection tracker for Android.
https://rethinkfirewall.com/
Apache License 2.0
2.84k stars 145 forks source link

tunnel: stop vpn adapter when another app takes over the vpn #1549

Closed SaadAhmedGit closed 1 month ago

SaadAhmedGit commented 3 months ago

fixes: #1545

This small change fixes the issue mentioned in the referenced issue. When another app takes over the VPN, onRevoke() method of the VpnService() is called which wasn't overriden to call stopVpnAdapter() which closes the tunnel.

Overriding onRevoke() to call stopVpnAdapter() ensures the tunnel is closed gracefully when another app starts the VPN.

ignoramous commented 3 months ago

Access to the variable vpnAdapter isn't synchronised, and so the check for vpnAdapter == null in the io coroutine is suspect.

Anyhow, thanks for the fix. I'll tag Hussain for review.

SaadAhmedGit commented 3 months ago

Access to the variable vpnAdapter isn't synchronised, and so the check for vpnAdapter == null in the io coroutine is suspect.

But super.onRevoke() only calls stopself() which doesn't use vpnAdapter. Even in signalStopService(), stopSelf() doesn't wait for stopVpnAdapter() to finish.

https://github.com/celzero/rethink-app/blob/26bb94a7cac16788859a93c57902760c3af630f6/app/src/main/java/com/celzero/bravedns/service/BraveVPNService.kt#L1926-L1931

ignoramous commented 3 months ago

vpnAdapter == null check is problematic regardless of what calls what, since one coroutine setting vpnAdapter to null will not be known to another coroutine unless there is a lock/serialisation (via Volatile).

Don't worry about it too much, we'll take a look at it as a separate issue.

SaadAhmedGit commented 1 month ago

Closing because already fixed.

Reference

hussainmohd-a commented 1 month ago

@SaadAhmedGit Thank you!!