mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-08 23:54:39 +00:00
fix(tor): audit fixes — first-bootstrap grace + tighten destroy() race
Audit of db378a1 surfaced three issues; this commit addresses them.
1) First-bootstrap self-heal storm (TorManager). On a fresh install with a
slow network the legitimate first bootstrap takes 30–60s. The 45s
stuck-Connecting watchdog used to fire resetWithCleanState, wiping an
empty state dir and adding a full bootstrap cycle of delay for no gain.
Now: track hasEverBootstrapped (flipped when status reaches Active);
pre-first-bootstrap self-heals use the gentler reset (drop client only,
keep state), post-first-bootstrap use resetWithCleanState. Wiping stale
on-disk guards only matters once we know Arti can actually work.
2) Rust destroy() race (lib.rs). The accept loop in startSocksProxy has no
.await between accept() returning and HANDLER_TASKS.push(h), so an
abort() alone is racy — a new handler can be spawned and pushed AFTER
our drain runs, which then holds an Arc<TorClient> past destroy() and
keeps the state file lock alive. Now: after abort(), await the SOCKS
JoinHandle with a 1s timeout so the listener fully terminates before
we drain HANDLER_TASKS. No new handlers can be added once the listener
is gone.
3) TOKIO_RUNTIME mutex held during block_on(sleep). The previous
`if let Some(rt) = TOKIO_RUNTIME.lock().unwrap().as_ref()` kept the
mutex held for the full sleep duration, blocking any other JNI caller
that needs the runtime. Now: clone the runtime Handle and release the
mutex immediately. Same fix applied to stopSocksProxy.
Rebuilds: libarti_android.so for arm64-v8a + x86_64.
This commit is contained in:
@@ -82,6 +82,16 @@ class TorManager(
|
||||
/** Wall-clock of the last automatic self-heal — rate-limits the stuck-Connecting reset. */
|
||||
@Volatile private var lastSelfHealAtMs: Long = 0L
|
||||
|
||||
/**
|
||||
* Flipped the first time [status] reaches [TorServiceStatus.Active] in this process. Before
|
||||
* that, the stuck-Connecting watchdog uses the gentler [TorService.reset] (drop client only)
|
||||
* rather than [TorService.resetWithCleanState] — because on a slow legitimate first
|
||||
* bootstrap there is no stale state to wipe, and wiping just forces an unnecessary
|
||||
* re-bootstrap cycle. Once we've seen Tor work once, persisted `arti/state/` is fair game
|
||||
* for the recovery to wipe.
|
||||
*/
|
||||
@Volatile private var hasEverBootstrapped: Boolean = false
|
||||
|
||||
init {
|
||||
scope.launch(Dispatchers.IO) {
|
||||
lastBypassApprovalMs = torPrefs.loadLastBypassApprovalMs()
|
||||
@@ -201,20 +211,36 @@ class TorManager(
|
||||
|
||||
init {
|
||||
// Self-heal watchdog. When status sits at Connecting for longer than
|
||||
// SELF_HEAL_AFTER_MS, the in-memory Arti state is almost certainly stuck —
|
||||
// bad guards, broken circuits, expired consensus. Drop the TorClient, wipe
|
||||
// on-disk state, and bump resetEpoch so the status combine re-fires and
|
||||
// re-enters the INTERNAL branch — which calls service.start() and, because
|
||||
// reset() flipped initialized=false, runs full Arti re-init with fresh
|
||||
// bootstrap. Rate-limited so a permanently broken network doesn't loop us.
|
||||
// Fires BEFORE the 60s connectionFailure dialog so most users never see it.
|
||||
// SELF_HEAL_AFTER_MS, the in-memory Arti state is likely stuck — bad guards,
|
||||
// broken circuits, expired consensus. Drop the TorClient and bump resetEpoch
|
||||
// so the status combine re-fires and re-enters the INTERNAL branch, which
|
||||
// runs full Arti re-init. Rate-limited so a permanently broken network
|
||||
// doesn't loop us. Fires BEFORE the 60s connectionFailure dialog so most
|
||||
// users never see it.
|
||||
//
|
||||
// Pre-first-bootstrap: gentle reset (drop client, keep state). On a slow
|
||||
// legitimate first bootstrap there's nothing on disk worth wiping, and
|
||||
// wiping just costs another full bootstrap cycle.
|
||||
// Post-first-bootstrap: full reset (drop client + wipe state). Once we've
|
||||
// seen Tor work once, a stuck Connecting almost certainly means stale on-disk
|
||||
// state from a different network needs to go.
|
||||
status
|
||||
.onEach {
|
||||
if (it is TorServiceStatus.Active) hasEverBootstrapped = true
|
||||
}.launchIn(scope)
|
||||
|
||||
selfHealSignal
|
||||
.onEach {
|
||||
val now = System.currentTimeMillis()
|
||||
if (now - lastSelfHealAtMs < SELF_HEAL_COOLDOWN_MS) return@onEach
|
||||
lastSelfHealAtMs = now
|
||||
Log.w("TorManager") { "Tor stuck Connecting >${SELF_HEAL_AFTER_MS}ms — self-healing (drop client + wipe state)" }
|
||||
service.resetWithCleanState()
|
||||
if (hasEverBootstrapped) {
|
||||
Log.w("TorManager") { "Tor stuck Connecting >${SELF_HEAL_AFTER_MS}ms — self-healing (drop client + wipe state)" }
|
||||
service.resetWithCleanState()
|
||||
} else {
|
||||
Log.w("TorManager") { "Tor stuck Connecting >${SELF_HEAL_AFTER_MS}ms on first bootstrap — self-healing (drop client only)" }
|
||||
service.reset()
|
||||
}
|
||||
resetEpoch.update { it + 1 }
|
||||
}.launchIn(scope)
|
||||
}
|
||||
|
||||
Binary file not shown.
Binary file not shown.
@@ -380,8 +380,13 @@ pub extern "C" fn Java_com_vitorpamplona_amethyst_ui_tor_ArtiNative_stopSocksPro
|
||||
handle.abort();
|
||||
}
|
||||
|
||||
if let Some(rt) = TOKIO_RUNTIME.lock().unwrap().as_ref() {
|
||||
rt.block_on(async {
|
||||
let rt_handle = TOKIO_RUNTIME
|
||||
.lock()
|
||||
.unwrap()
|
||||
.as_ref()
|
||||
.map(|rt| rt.handle().clone());
|
||||
if let Some(rh) = rt_handle {
|
||||
rh.block_on(async {
|
||||
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
|
||||
});
|
||||
}
|
||||
@@ -405,9 +410,26 @@ pub extern "C" fn Java_com_vitorpamplona_amethyst_ui_tor_ArtiNative_destroy(
|
||||
) -> jint {
|
||||
log_info!("Destroying Arti client");
|
||||
|
||||
// Abort the listener task first so no new handlers spawn.
|
||||
if let Some(handle) = SOCKS_TASK.lock().unwrap().take() {
|
||||
handle.abort();
|
||||
// Clone the runtime handle and release the TOKIO_RUNTIME mutex immediately —
|
||||
// we will hold it for ~500ms below, and other JNI calls that need the runtime
|
||||
// (e.g. a Kotlin start() racing with us) would otherwise block on this mutex.
|
||||
let rt_handle = TOKIO_RUNTIME
|
||||
.lock()
|
||||
.unwrap()
|
||||
.as_ref()
|
||||
.map(|rt| rt.handle().clone());
|
||||
|
||||
// Abort the listener and wait for it to actually terminate before draining
|
||||
// HANDLER_TASKS. The accept loop has no .await between `accept` and
|
||||
// `HANDLER_TASKS.push(h)`, so abort() alone is racy: a handler can be spawned
|
||||
// and pushed AFTER our drain. Awaiting the JoinHandle (with timeout) closes
|
||||
// that window — no new handlers can be pushed once the listener task is gone.
|
||||
let socks_handle = SOCKS_TASK.lock().unwrap().take();
|
||||
if let (Some(h), Some(rh)) = (socks_handle, rt_handle.as_ref()) {
|
||||
h.abort();
|
||||
rh.block_on(async {
|
||||
let _ = tokio::time::timeout(tokio::time::Duration::from_secs(1), h).await;
|
||||
});
|
||||
}
|
||||
|
||||
// Abort all in-flight handlers — each holds an Arc<TorClient> clone, and
|
||||
@@ -421,8 +443,8 @@ pub extern "C" fn Java_com_vitorpamplona_amethyst_ui_tor_ArtiNative_destroy(
|
||||
|
||||
// Give tokio a moment to actually cancel and drop the task frames so the
|
||||
// handler Arcs are released before we drop our static one.
|
||||
if let Some(rt) = TOKIO_RUNTIME.lock().unwrap().as_ref() {
|
||||
rt.block_on(async {
|
||||
if let Some(rh) = rt_handle {
|
||||
rh.block_on(async {
|
||||
tokio::time::sleep(tokio::time::Duration::from_millis(500)).await;
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user