From 3562ad10d53f842604b5801b0d2057da8c335f18 Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Thu, 30 Jul 2026 00:50:36 +0300 Subject: [PATCH 1/4] fix(wifi-aware): count hotspot radio holds and never drop a restart request Two fixes to how Wi-Fi Aware yields the radio to the Wi-Fi Direct hotspot. A restart request could be swallowed: restartIfStillEnabled() coalesces on an in-flight flag, so a request arriving while an earlier loop was still burning attempts against the hotspot hold lost the CAS and was dropped. The loop then exhausted its attempts without ever seeing the cleared hold, leaving the mesh down despite the user's setting. Requests are now recorded before coalescing and re-checked after each pass. The hold itself was a single flag, so overlapping share sessions could release each other's claim. It is now a counted, once-releasable HotspotLease: Aware restarts only when no session still needs the radio, and a duplicate or late release is a no-op. Publication of a started service is checked against the hold inside the same lock stop() takes, so a start racing a new hold cannot resurrect NAN while the hotspot owns the radio. Co-Authored-By: Claude Opus 5 (1M context) --- .../android/wifi-aware/WifiAwareController.kt | 132 ++++++++++++++---- .../wifiaware/WifiAwareHotspotHoldTest.kt | 73 ++++++++++ 2 files changed, 175 insertions(+), 30 deletions(-) create mode 100644 app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareHotspotHoldTest.kt diff --git a/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt b/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt index ccb8b55f..d5a694f1 100644 --- a/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt +++ b/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt @@ -11,7 +11,9 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch +import androidx.annotation.VisibleForTesting import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicInteger /** * WifiAwareController manages lifecycle and debug surfacing for the WifiAwareMeshService. @@ -27,17 +29,29 @@ object WifiAwareController { private val lifecycleLock = Any() private var starting = false private val restartInFlight = AtomicBoolean(false) + private val restartRequested = AtomicBoolean(false) private var awareReceiverRegistered = false private var lastBlockedReason: String? = null /** - * Set while a Wi-Fi Direct hotspot is hosting. Wi-Fi Aware (NAN) and Wi-Fi Direct - * (P2P) cannot hold interfaces at the same time on common chipsets — the HAL fails - * to create the P2P iface and every createGroup is answered with BUSY. The hold - * also blocks [startIfPossible], so a resume or mesh-service restart cannot bring - * Aware back while the hotspot is up. + * How many hotspot sessions currently need the radio. + * + * Wi-Fi Aware (NAN) and Wi-Fi Direct (P2P) cannot hold interfaces at the same time on + * common chipsets — the HAL fails to create the P2P iface and every createGroup is + * answered with BUSY. A non-zero count also blocks [startIfPossible], so a resume or + * mesh-service restart cannot bring Aware back while a hotspot is up. + * + * A count rather than a flag because teardown is asynchronous and sessions can + * overlap. Callers receive a once-releasable [HotspotLease], so one session cannot + * accidentally consume another session's hold. */ - private val hotspotHold = AtomicBoolean(false) + private val hotspotHolds = AtomicInteger(0) + + /** True while any hotspot session still needs the radio. */ + @VisibleForTesting + internal fun isHeldForHotspot(): Boolean = hotspotHolds.get() > 0 + + private fun heldForHotspot(): Boolean = isHeldForHotspot() private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) @@ -97,26 +111,65 @@ object WifiAwareController { } /** - * Releases the Wi-Fi radio so a Wi-Fi Direct hotspot can create its P2P interface, - * and prevents Aware restarting until [releaseHotspotHold] is called. + * A session-scoped claim on the Wi-Fi radio. + * + * Closing the same lease twice is a no-op; raw decrements are intentionally not + * exposed because lifecycle retries and duplicate teardown calls must not release a + * different hotspot session's hold. */ - fun holdForHotspot() { - if (!hotspotHold.compareAndSet(false, true)) return - Log.i(TAG, "Holding Wi-Fi Aware down so the hotspot can use the radio") - stop() + class HotspotLease internal constructor( + private val releaseAction: () -> Unit + ) : AutoCloseable { + private val released = AtomicBoolean(false) + + override fun close() { + if (released.compareAndSet(false, true)) { + releaseAction() + } + } } - /** Drops the hold and restores Aware if the user still has it enabled. */ - fun releaseHotspotHold() { - if (!hotspotHold.compareAndSet(true, false)) return + /** + * Releases the Wi-Fi radio so a Wi-Fi Direct hotspot can create its P2P interface, + * and prevents Aware restarting until the returned lease is closed. + * + * Counted because a second share can start while the first is still cleaning up. + */ + fun acquireHotspotLease(): HotspotLease { + if (hotspotHolds.getAndIncrement() == 0) { + Log.i(TAG, "Holding Wi-Fi Aware down so the hotspot can use the radio") + stop() + } + return HotspotLease(::releaseHotspotLease) + } + + private fun releaseHotspotLease() { + var remaining: Int + while (true) { + val current = hotspotHolds.get() + if (current == 0) return + remaining = current - 1 + if (hotspotHolds.compareAndSet(current, remaining)) break + } + + if (remaining != 0) { + Log.d(TAG, "Hotspot finished, but $remaining other hold(s) remain") + return + } + Log.i(TAG, "Hotspot finished; restoring Wi-Fi Aware if enabled") restartIfStillEnabled() } + @VisibleForTesting + internal fun resetHotspotLeasesForTest() { + hotspotHolds.set(0) + } + fun startIfPossible() { val reusableService = synchronized(lifecycleLock) { if (!_enabled.value) return - if (hotspotHold.get()) { + if (heldForHotspot()) { Log.d(TAG, "Not starting Wi-Fi Aware: held down for the hotspot") return } @@ -179,7 +232,7 @@ object WifiAwareController { return } } - if (!_enabled.value || hotspotHold.get()) { + if (!_enabled.value || heldForHotspot()) { synchronized(lifecycleLock) { starting = false } return } @@ -191,13 +244,13 @@ object WifiAwareController { startedService.startServices() // Test the hold inside the same lock that publishes the service, and that - // stop() takes. Testing it outside leaves a window where holdForHotspot() + // stop() takes. Testing it outside leaves a window where acquiring a lease // sets the flag and stop() finds nothing published yet, and this block then // publishes anyway — resurrecting NAN while the hotspot owns the radio. - // Ordering holds because holdForHotspot() sets the flag before calling + // Ordering holds because acquireHotspotLease() increments before calling // stop(): either we see the flag here, or stop() sees our published service. val published = synchronized(lifecycleLock) { - val canPublish = !hotspotHold.get() && startedService.isRunning() + val canPublish = !heldForHotspot() && startedService.isRunning() if (canPublish) { service = startedService _running.value = true @@ -214,7 +267,7 @@ object WifiAwareController { try { com.bitchat.android.ui.debug.DebugSettingsManager.getInstance().addDebugMessage(com.bitchat.android.ui.debug.DebugMessage.SystemMessage("Wi‑Fi Aware started")) } catch (_: Exception) {} } else { // stopServices() can block, so keep it out of the lock. - val heldForHotspot = hotspotHold.get() + val heldForHotspot = heldForHotspot() if (heldForHotspot || reusableService == null) { try { startedService.stopServices() } catch (_: Exception) { } } @@ -264,6 +317,13 @@ object WifiAwareController { * startServices() defers and we must try again rather than give up. */ internal fun restartIfStillEnabled(delayMs: Long = 0L) { + // Record the request before coalescing. A request that arrives while a loop is + // already running — the final lease closing during a loop that has been + // burning attempts against the hold — would otherwise be dropped, and the old + // loop would exhaust MAX_RESTART_ATTEMPTS without ever seeing the cleared hold, + // leaving Aware down despite the user's setting. + restartRequested.set(true) + if (!restartInFlight.compareAndSet(false, true)) { Log.d(TAG, "Restart already in flight; coalescing request") return @@ -271,18 +331,30 @@ object WifiAwareController { scope.launch { try { if (delayMs > 0L) delay(delayMs) - var attempt = 0 - while (_enabled.value && !_running.value && attempt < MAX_RESTART_ATTEMPTS) { - val ctx = appContext - if (ctx != null && !refreshSupportStatus(ctx).supported) break - startIfPossible() - if (_running.value) break - attempt++ - delay(RESTART_RETRY_DELAY_MS) - } + do { + // Consume the request before working, so anything arriving during + // the attempts below schedules another pass. + restartRequested.set(false) + var attempt = 0 + while (_enabled.value && !_running.value && attempt < MAX_RESTART_ATTEMPTS) { + val ctx = appContext + if (ctx != null && !refreshSupportStatus(ctx).supported) break + startIfPossible() + if (_running.value) break + attempt++ + delay(RESTART_RETRY_DELAY_MS) + } + } while (restartRequested.get() && _enabled.value && !_running.value) } finally { restartInFlight.set(false) } + + // A request landing between the loop exiting and the flag clearing finds + // restartInFlight still set and returns; pick it up here. Terminates because + // each pass consumes the flag before doing any work. + if (restartRequested.get() && _enabled.value && !_running.value) { + restartIfStillEnabled() + } } } diff --git a/app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareHotspotHoldTest.kt b/app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareHotspotHoldTest.kt new file mode 100644 index 00000000..7805b9ad --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareHotspotHoldTest.kt @@ -0,0 +1,73 @@ +package com.bitchat.android.wifiaware + +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * The hold keeps Wi-Fi Aware off the radio while a hotspot uses it. Getting the + * bookkeeping wrong is not cosmetic: release it early and the P2P group cannot form, + * fail to release it and the mesh stays down. + * + * Teardown is asynchronous, so sessions overlap and releases arrive out of order. These + * cover the counting rules that make that safe. + */ +@RunWith(RobolectricTestRunner::class) +class WifiAwareHotspotHoldTest { + + @Before + fun clearAnyLeftoverHolds() { + WifiAwareController.resetHotspotLeasesForTest() + } + + @After + fun clearTestHolds() { + WifiAwareController.resetHotspotLeasesForTest() + } + + @Test + fun `a single session holds and then releases`() { + val lease = WifiAwareController.acquireHotspotLease() + assertTrue(WifiAwareController.isHeldForHotspot()) + + lease.close() + assertFalse(WifiAwareController.isHeldForHotspot()) + } + + @Test + fun `a second session starting before the first finishes keeps the radio held`() { + val first = WifiAwareController.acquireHotspotLease() + val second = WifiAwareController.acquireHotspotLease() + + // The first session's asynchronous teardown completes. + first.close() + + assertTrue( + "the second session still needs the radio", + WifiAwareController.isHeldForHotspot() + ) + + second.close() + assertFalse(WifiAwareController.isHeldForHotspot()) + } + + @Test + fun `closing one lease twice cannot release another session`() { + val first = WifiAwareController.acquireHotspotLease() + val second = WifiAwareController.acquireHotspotLease() + + first.close() + first.close() + assertTrue(WifiAwareController.isHeldForHotspot()) + + second.close() + assertFalse( + "only the matching session can release its radio claim", + WifiAwareController.isHeldForHotspot() + ) + } +} From 141adf546812d2ed9ba54c54fdfbc78575646934 Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Thu, 30 Jul 2026 00:50:36 +0300 Subject: [PATCH 2/4] fix(hotspot): only remove our own group; ask consent to replace a foreign one removeGroup() is device-scoped: it removes whatever Wi-Fi Direct group exists, including one owned by Cast, Android Auto or Quick Share. stopHotspot() called it unconditionally, so the path built to protect a foreign group tore that group down anyway. Removal on stop is now gated on a createdGroup flag, set once our own createGroup command is accepted; with nothing of ours on the framework, stop closes the channel and leaves the group alone. When a group we did not record creating is active at start, the app no longer guesses about ownership - it asks. A confirmation dialog explains that starting will disconnect the current Wi-Fi Direct connection; confirming retries the start with replacement authorized, cancelling leaves everything untouched. Consent is bound to the group it was given for: the conflicting group's name travels through the dialog, and the policy only authorizes removing a group with exactly that name - one that appeared later, or swapped in mid-retry, re-prompts instead of riding on stale approval. Because consent replaces ownership proof, the DIRECT-BC- prefix heuristic is gone: a prefix match is not ownership (this device can be connected to another phone's bitchat group), so only the exact recorded name counts. The record is also kept honest: never taken from a group we do not host, and never overwritten while an old group of ours may still exist, so a BUSY retry cannot misclassify our own stale group as foreign. Also: SecurityException guards on the removeGroup() sites reached from framework callbacks (permission revoked mid-session crashed instead of failing cleanly); stopHotspot() takes a completion callback so the ViewModel releases its Wi-Fi Aware lease only after the framework acknowledges the removal, with an idempotent 10s fallback so a dropped acknowledgement cannot pin the mesh down; and the confirm/cancel handlers guard on the ConfirmDisconnect state so a tap landing through a screen transition cannot tear down a just-confirmed session. Replaces the state-machine approach of #811 - same protection at proportionate cost. Co-Authored-By: Claude Opus 5 (1M context) --- .../android/hotspot/HotspotActivity.kt | 30 ++++ .../bitchat/android/hotspot/HotspotManager.kt | 168 ++++++++++++++---- .../android/hotspot/HotspotStartupPolicy.kt | 29 ++- .../android/hotspot/HotspotViewModel.kt | 91 +++++++++- app/src/main/res/values/strings.xml | 3 + .../hotspot/HotspotStartupPolicyTest.kt | 87 +++++++-- 6 files changed, 342 insertions(+), 66 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotActivity.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotActivity.kt index bf9745d9..35d0f4ec 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotActivity.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotActivity.kt @@ -28,12 +28,14 @@ import androidx.compose.ui.graphics.asImageBitmap import androidx.compose.ui.platform.LocalClipboardManager import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.platform.LocalDensity +import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.AnnotatedString import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp import androidx.lifecycle.compose.collectAsStateWithLifecycle +import com.bitchat.android.R import com.bitchat.android.ui.theme.BitchatFontFamily import com.bitchat.android.ui.theme.BitchatTheme import com.bitchat.android.util.UniversalApkManager @@ -137,6 +139,12 @@ fun HotspotScreen( is HotspotViewModel.HotspotState.Starting -> { LoadingScreen() } + is HotspotViewModel.HotspotState.ConfirmDisconnect -> { + ExistingGroupConfirmation( + onConfirm = viewModel::confirmDisconnectAndStart, + onCancel = viewModel::cancelDisconnect + ) + } is HotspotViewModel.HotspotState.Active -> { ActiveHotspotScreen(state = currentState) } @@ -152,6 +160,28 @@ fun HotspotScreen( } } +@Composable +private fun ExistingGroupConfirmation( + onConfirm: () -> Unit, + onCancel: () -> Unit +) { + AlertDialog( + onDismissRequest = onCancel, + title = { Text(stringResource(R.string.hotspot_disconnect_title)) }, + text = { Text(stringResource(R.string.hotspot_disconnect_message)) }, + confirmButton = { + TextButton(onClick = onConfirm) { + Text(stringResource(R.string.hotspot_disconnect_confirm)) + } + }, + dismissButton = { + TextButton(onClick = onCancel) { + Text(stringResource(R.string.cancel)) + } + } + ) +} + @OptIn(ExperimentalPermissionsApi::class) @Composable fun IntroScreen(onStartHotspot: () -> Unit) { diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt index 8f503fb5..5f30b8ec 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt @@ -63,6 +63,16 @@ class HotspotManager(private val context: Context) { private var hasNotifiedStarted = false // Track if we've notified the callback private var isReceiverRegistered = false // Track receiver registration to prevent leaks + // Set once our own createGroup command is accepted. stopHotspot() only calls + // removeGroup() when this is true: removal is device-scoped, so issuing it with + // nothing of ours on the framework could only tear down another app's session + // (Cast, Android Auto, Quick Share). + private var createdGroup = false + + // Name of the foreign group the user explicitly agreed to disconnect, or null. + // Consent is per-group: a group with a different name asks again. + private var confirmedReplacementName: String? = null + // Saved credentials for reconnection private var savedSsid: String? = null private var savedPassword: String? = null @@ -103,8 +113,13 @@ class HotspotManager(private val context: Context) { /** * Start the Wi-Fi P2P hotspot. + * + * @param confirmedReplacementName name of the foreign Wi-Fi Direct group the + * user has confirmed may be disconnected, as previously reported through + * [HotspotCallback.onExistingGroupConflict]. When null (or when the group + * present no longer matches), a foreign group is reported instead of touched. */ - fun startHotspot(callback: HotspotCallback) { + fun startHotspot(callback: HotspotCallback, confirmedReplacementName: String? = null) { if (isStarting) { Log.w(TAG, "Hotspot already starting") return @@ -129,6 +144,7 @@ class HotspotManager(private val context: Context) { } this.callback = callback + this.confirmedReplacementName = confirmedReplacementName isStarting = true Log.d(TAG, "Starting Wi-Fi P2P hotspot") @@ -162,8 +178,12 @@ class HotspotManager(private val context: Context) { /** * Stop the hotspot. + * + * @param onTeardownComplete invoked once the framework has acknowledged the + * removal of our group (or immediately when this session created none). Lets + * the caller hold the Wi-Fi Aware radio back until the P2P group is gone. */ - fun stopHotspot() { + fun stopHotspot(onTeardownComplete: (() -> Unit)? = null) { Log.d(TAG, "Stopping hotspot") isStarting = false @@ -177,19 +197,40 @@ class HotspotManager(private val context: Context) { val staleChannel = channel channel = null - if (staleChannel != null) { - wifiP2pManager?.removeGroup(staleChannel, object : ActionListener { - override fun onSuccess() { - Log.d(TAG, "Group removed successfully") - // Nothing of ours is left for a later run to clean up. - ownedGroupName = null - closeChannel(staleChannel) - } - override fun onFailure(reason: Int) { - Log.w(TAG, "Failed to remove group: $reason") - closeChannel(staleChannel) - } - }) + val hadOwnGroup = createdGroup + createdGroup = false + + if (staleChannel != null && hadOwnGroup) { + try { + wifiP2pManager?.removeGroup(staleChannel, object : ActionListener { + override fun onSuccess() { + Log.d(TAG, "Group removed successfully") + // Nothing of ours is left for a later run to clean up. + ownedGroupName = null + closeChannel(staleChannel) + onTeardownComplete?.invoke() + } + override fun onFailure(reason: Int) { + Log.w(TAG, "Failed to remove group: $reason") + closeChannel(staleChannel) + onTeardownComplete?.invoke() + } + }) + } catch (e: SecurityException) { + // Revoked mid-session. Closing the channel still detaches this app's + // binder, which asks the framework to drop our group with it. + Log.e(TAG, "Wi-Fi permission was revoked while removing the group", e) + closeChannel(staleChannel) + onTeardownComplete?.invoke() + } + } else if (staleChannel != null) { + // This session created nothing, so there is nothing of ours to remove. + // removeGroup() here is exactly the bug this change fixes: device-scoped + // removal would disconnect whatever group another app has running. + closeChannel(staleChannel) + onTeardownComplete?.invoke() + } else { + onTeardownComplete?.invoke() } // Release locks @@ -305,7 +346,8 @@ class HotspotManager(private val context: Context) { val action = HotspotStartupPolicy.startAction( p2pState = lastP2pState, existingGroupName = existingGroup?.networkName, - ownedGroupName = ownedGroupName + ownedGroupName = ownedGroupName, + confirmedGroupName = confirmedReplacementName ) when (action) { @@ -313,7 +355,19 @@ class HotspotManager(private val context: Context) { Log.w(TAG, "Not attempting group creation: ${action.message}") failStartup(action.message) } - HotspotStartupPolicy.StartAction.Create -> createGroup(attempt) + HotspotStartupPolicy.StartAction.ConfirmReplaceExisting -> { + val name = existingGroup?.networkName + if (name == null) { + // Unreachable while the policy requires a name, but there + // is nothing safe to bind consent to without one. + failStartup(HotspotStartupPolicy.P2P_BUSY_MESSAGE) + } else { + Log.i(TAG, "Existing group '$name' is not ours; asking the user") + reportExistingGroupConflict(name) + } + } + HotspotStartupPolicy.StartAction.Create -> + createGroup(attempt, oldGroupCleared = true) HotspotStartupPolicy.StartAction.RemoveStaleGroupThenCreate -> { Log.w(TAG, "Removing stale group '${existingGroup?.networkName}' before creating") removeStaleGroup(ch, attempt) @@ -327,33 +381,46 @@ class HotspotManager(private val context: Context) { } private fun removeStaleGroup(ch: Channel, attempt: Int) { - wifiP2pManager?.removeGroup(ch, object : ActionListener { - override fun onSuccess() { - if (channel !== ch) return - Log.d(TAG, "Stale group removed") - createGroup(attempt) - } - override fun onFailure(reason: Int) { - if (channel !== ch) return - // Creation may still succeed, and a BUSY reply here backs off as usual. - Log.w(TAG, "Failed to remove stale group: $reason; attempting creation anyway") - createGroup(attempt) - } - }) + try { + wifiP2pManager?.removeGroup(ch, object : ActionListener { + override fun onSuccess() { + if (channel !== ch) return + Log.d(TAG, "Stale group removed") + createGroup(attempt, oldGroupCleared = true) + } + override fun onFailure(reason: Int) { + if (channel !== ch) return + // Creation may still succeed, and a BUSY reply here backs off as usual. + Log.w(TAG, "Failed to remove stale group: $reason; attempting creation anyway") + createGroup(attempt, oldGroupCleared = false) + } + }) + } catch (e: SecurityException) { + Log.e(TAG, "Wi-Fi permission was revoked while removing the existing group", e) + failStartup("A required Wi-Fi or local network permission was revoked. Grant it and try again.") + } } /** * Create Wi-Fi P2P group. + * + * @param oldGroupCleared false when a previous group may still exist (its removal + * just failed). The ownership marker keeps the OLD group's name in that case: + * overwriting it early would make a BUSY retry classify our own stale group as + * foreign and raise a spurious consent dialog. On success the group-info poll + * records the authoritative name anyway. */ @SuppressLint("MissingPermission") - private fun createGroup(attempt: Int) { + private fun createGroup(attempt: Int, oldGroupCleared: Boolean) { val ch = channel ?: return try { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { // Record before the call: if the process dies between creation and the // first group info, the next run still knows this orphan is ours. - ownedGroupName = savedSsid + if (oldGroupCleared) { + ownedGroupName = savedSsid + } // Android 10+: Custom SSID and password val config = WifiP2pConfig.Builder() @@ -376,11 +443,18 @@ class HotspotManager(private val context: Context) { private fun groupActionListener(attempt: Int, requestChannel: Channel) = object : ActionListener { override fun onSuccess() { if (channel !== requestChannel) { + // Ours by construction: this listener only observes our own createGroup. Log.w(TAG, "Removing group created after hotspot was stopped") - wifiP2pManager?.removeGroup(requestChannel, null) + try { + wifiP2pManager?.removeGroup(requestChannel, null) + } catch (e: SecurityException) { + // The orphan stays; the next start recognises it via ownedGroupName. + Log.e(TAG, "Could not remove the late group; permission was revoked", e) + } return } Log.d(TAG, "P2P group created successfully") + createdGroup = true isStarting = false // Don't call onHotspotStarted() yet - wait for group info startGroupInfoPolling() @@ -434,6 +508,17 @@ class HotspotManager(private val context: Context) { cb?.onError(message) } + /** + * A group belonging to another app is up. Stop cleanly — with [createdGroup] + * false the stop path leaves that group untouched — and let the UI ask whether + * starting the hotspot may disconnect it. + */ + private fun reportExistingGroupConflict(groupName: String) { + val cb = callback + stopHotspot() + cb?.onExistingGroupConflict(groupName) + } + /** * Start polling for group info to track connected clients. */ @@ -481,8 +566,13 @@ class HotspotManager(private val context: Context) { savedPassword = group.passphrase } - // Authoritative name straight from the framework - group.networkName?.let { ownedGroupName = it } + // Authoritative name straight from the framework. Only recorded + // for a group we host: in the client role the group is another + // app's, and recording its name would make the next run treat a + // foreign orphan as ours. + if (group.isGroupOwner) { + group.networkName?.let { ownedGroupName = it } + } // Notify callback on FIRST successful group info retrieval if (!hasNotifiedStarted) { @@ -618,6 +708,14 @@ class HotspotManager(private val context: Context) { interface HotspotCallback { fun onHotspotStarted() fun onConnectionInfoUpdated(info: ConnectionInfo?) + + /** + * A Wi-Fi Direct group belonging to another app is active and the caller has + * not confirmed replacing it. Ask the user, then retry with this name as + * `confirmedReplacementName` if they accept. Nothing was disturbed. + */ + fun onExistingGroupConflict(groupName: String) + fun onError(message: String) } } diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt index 48284d0a..3e9f32d8 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt @@ -19,8 +19,6 @@ internal object HotspotStartupPolicy { const val SSID_PREFIX = "DIRECT-BC-" // BC for BitChat const val P2P_UNSUPPORTED_MESSAGE = "Wi-Fi Direct is not supported on this device." - const val FOREIGN_GROUP_MESSAGE = - "Another app is using Wi-Fi Direct. Close it and try again." const val P2P_BUSY_MESSAGE = "Wi-Fi Direct is busy. Please try again in a moment." const val GENERIC_FAILURE_MESSAGE = "Failed to start the hotspot. Please try again." @@ -32,6 +30,10 @@ internal object HotspotStartupPolicy { sealed interface StartAction { data object Create : StartAction data object RemoveStaleGroupThenCreate : StartAction + + /** A group we cannot show is ours is up; ask the user before disturbing it. */ + data object ConfirmReplaceExisting : StartAction + data class Fail(val message: String) : StartAction } @@ -42,29 +44,38 @@ internal object HotspotStartupPolicy { * hosting leaves an orphan behind. The framework rejects createGroup with BUSY * while any group exists, and no amount of retrying clears it. * - * Wi-Fi Direct is shared with Cast, Android Auto and Quick Share, so only groups - * we can show are ours get torn down. + * Wi-Fi Direct is shared with Cast, Android Auto and Quick Share. A group we can + * show is ours is removed silently; anything else needs the user's explicit + * go-ahead before it is touched. Consent is bound to the group it was given + * for: a group with any other name — one that appeared after the dialog, or + * mid-retry — asks again instead of riding on stale approval. * * @param existingGroupName network name of the group already present, or null * @param ownedGroupName last group name this app recorded creating, or null + * @param confirmedGroupName group the user agreed to disconnect, or null */ fun startAction( p2pState: Int?, existingGroupName: String?, - ownedGroupName: String? + ownedGroupName: String?, + confirmedGroupName: String? ): StartAction = when { p2pState == WifiP2pManager.WIFI_P2P_STATE_DISABLED -> StartAction.Fail(P2P_DISABLED_MESSAGE) existingGroupName == null -> StartAction.Create isOurs(existingGroupName, ownedGroupName) -> StartAction.RemoveStaleGroupThenCreate - else -> StartAction.Fail(FOREIGN_GROUP_MESSAGE) + existingGroupName == confirmedGroupName -> StartAction.RemoveStaleGroupThenCreate + else -> StartAction.ConfirmReplaceExisting } /** - * Primary signal is the name we recorded creating. The SSID prefix is only a - * fallback, covering orphans left by builds that predate that record. + * Only the exact name this device recorded creating counts as ours. A prefix + * match is not ownership: this device can be connected to another phone's + * bitchat group — same prefix, their suffix — and silently removing it would + * disconnect that session. An orphan predating the record simply goes through + * the confirmation dialog once. */ private fun isOurs(existingGroupName: String, ownedGroupName: String?): Boolean = - existingGroupName == ownedGroupName || existingGroupName.startsWith(SSID_PREFIX) + ownedGroupName != null && existingGroupName == ownedGroupName /** * @param reason a [WifiP2pManager] failure reason from `ActionListener.onFailure` diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotViewModel.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotViewModel.kt index 581b1d99..73f9faa4 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotViewModel.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotViewModel.kt @@ -1,6 +1,8 @@ package com.bitchat.android.hotspot import android.app.Application +import android.os.Handler +import android.os.Looper import android.util.Log import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope @@ -18,6 +20,10 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) companion object { private const val TAG = "HotspotViewModel" + + // Upper bound on waiting for the framework to acknowledge group removal + // before the Wi-Fi Aware lease is released anyway. + private const val TEARDOWN_FALLBACK_MILLIS = 10_000L } private val _state = MutableStateFlow(HotspotState.Intro) @@ -25,12 +31,25 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) private var hotspotManager: HotspotManager? = null private var webServer: ApkWebServer? = null + + /** APK waiting on the user's answer to the disconnect confirmation. */ + private var pendingApk: File? = null + + /** Group the pending confirmation is about; consent binds to this name only. */ + private var pendingGroupName: String? = null + + /** Once-releasable radio claim owned by the current hotspot session. */ + private var awareLease: WifiAwareController.HotspotLease? = null private val context = application.applicationContext /** * Start the hotspot with the provided APK file. */ fun startHotspot(apkFile: File) { + startHotspot(apkFile, confirmedGroupName = null) + } + + private fun startHotspot(apkFile: File, confirmedGroupName: String?) { if (_state.value is HotspotState.Starting || _state.value is HotspotState.Active) { Log.w(TAG, "Hotspot already starting or active") return @@ -43,7 +62,7 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) try { // Wi-Fi Aware holds a NAN interface that blocks the P2P one; release it // first or every createGroup comes back BUSY. Restored when we stop. - WifiAwareController.holdForHotspot() + awareLease = WifiAwareController.acquireHotspotLease() // Start hotspot val manager = HotspotManager(context) @@ -94,10 +113,21 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) } } + override fun onExistingGroupConflict(groupName: String) { + viewModelScope.launch { + // Nothing was disturbed; the manager already stopped + // itself. Release our resources and ask the user. + teardown() + pendingApk = apkFile + pendingGroupName = groupName + _state.value = HotspotState.ConfirmDisconnect + } + } + override fun onError(message: String) { viewModelScope.launch { failWith(message) } } - }) + }, confirmedGroupName) } catch (e: Exception) { Log.e(TAG, "Error starting hotspot", e) @@ -106,11 +136,34 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) } } + /** The user agreed that starting may disconnect the existing Wi-Fi Direct group. */ + fun confirmDisconnectAndStart() { + if (_state.value !is HotspotState.ConfirmDisconnect) return + val apk = pendingApk ?: return + val groupName = pendingGroupName + pendingApk = null + pendingGroupName = null + startHotspot(apk, confirmedGroupName = groupName) + } + + fun cancelDisconnect() { + // A tap landing late (e.g. through an exit animation) must not tear down + // the session a just-processed confirmation is starting. + if (_state.value !is HotspotState.ConfirmDisconnect) return + pendingApk = null + pendingGroupName = null + // The conflict path already tore down; this only covers a stray state. + teardown() + _state.value = HotspotState.Intro + } + /** * Stop the hotspot and web server. */ fun stopHotspot() { Log.d(TAG, "Stopping hotspot") + pendingApk = null + pendingGroupName = null teardown() _state.value = HotspotState.Intro } @@ -125,6 +178,8 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) */ private fun failWith(message: String) { Log.e(TAG, "Hotspot failed: $message") + pendingApk = null + pendingGroupName = null teardown() _state.value = HotspotState.Error(message) } @@ -134,10 +189,34 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) webServer?.stopServer() webServer = null - hotspotManager?.stopHotspot() + val manager = hotspotManager hotspotManager = null - WifiAwareController.releaseHotspotHold() + // Nothing of ours to hand back. An earlier teardown may already have passed + // its once-releasable lease to the manager's completion callback. + val lease = awareLease ?: return + awareLease = null + + if (manager == null) { + lease.close() + return + } + + // Close the lease only when the manager has finished removing our group. + // Restoring Wi-Fi Aware earlier recreates the NAN/P2P radio contention the + // lease exists to prevent. The lease is idempotent, so a duplicate or late + // completion cannot release a newer session's claim. + manager.stopHotspot { lease.close() } + + // A framework that never acknowledges the removal must not pin Wi-Fi Aware + // down for the life of the process. close() is idempotent and this lease + // belongs to this session alone, so whichever path runs second is a no-op. + // A plain handler rather than viewModelScope: onCleared() cancels the scope + // right when this teardown may be running. + Handler(Looper.getMainLooper()).postDelayed( + { lease.close() }, + TEARDOWN_FALLBACK_MILLIS + ) } /** @@ -160,6 +239,10 @@ class HotspotViewModel(application: Application) : AndroidViewModel(application) sealed class HotspotState { object Intro : HotspotState() object Starting : HotspotState() + + /** A foreign Wi-Fi Direct group is up; waiting for the user's go-ahead. */ + object ConfirmDisconnect : HotspotState() + data class Active( val ssid: String, val password: String, diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 18762913..88c495c1 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -275,6 +275,9 @@ Create Wi-Fi hotspot to share offline Share via Quick Share Use standard Android sharing + Disconnect current Wi-Fi Direct connection? + Starting the hotspot will disconnect the current Wi-Fi Direct connection. This may interrupt Cast, Android Auto, Quick Share, or another nearby connection. + Disconnect and start Install Received APK diff --git a/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt b/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt index bb91b072..a33577a3 100644 --- a/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt @@ -76,49 +76,83 @@ class HotspotStartupPolicyTest { val action = HotspotStartupPolicy.startAction( p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, existingGroupName = "DIRECT-BC-CUF6EN63", - ownedGroupName = "DIRECT-BC-CUF6EN63" + ownedGroupName = "DIRECT-BC-CUF6EN63", + confirmedGroupName = null ) assertEquals(HotspotStartupPolicy.StartAction.RemoveStaleGroupThenCreate, action) } @Test - fun `another app's group is left alone`() { + fun `another app's group requires the user's confirmation`() { val action = HotspotStartupPolicy.startAction( p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, existingGroupName = "DIRECT-xY-Chromecast", - ownedGroupName = "DIRECT-BC-CUF6EN63" + ownedGroupName = "DIRECT-BC-CUF6EN63", + confirmedGroupName = null ) - assertEquals( - HotspotStartupPolicy.StartAction.Fail(HotspotStartupPolicy.FOREIGN_GROUP_MESSAGE), - action - ) + assertEquals(HotspotStartupPolicy.StartAction.ConfirmReplaceExisting, action) } @Test - fun `an orphan from before we recorded ownership is still recognised by its prefix`() { + fun `a confirmed replacement removes the confirmed foreign group`() { + val action = HotspotStartupPolicy.startAction( + p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, + existingGroupName = "DIRECT-xY-Chromecast", + ownedGroupName = null, + confirmedGroupName = "DIRECT-xY-Chromecast" + ) + + assertEquals(HotspotStartupPolicy.StartAction.RemoveStaleGroupThenCreate, action) + } + + @Test + fun `consent for one group does not authorize removing a different one`() { + val action = HotspotStartupPolicy.startAction( + p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, + existingGroupName = "DIRECT-aB-QuickShare", + ownedGroupName = null, + confirmedGroupName = "DIRECT-xY-Chromecast" + ) + + assertEquals(HotspotStartupPolicy.StartAction.ConfirmReplaceExisting, action) + } + + @Test + fun `an unrecorded orphan needs confirmation even with our prefix`() { val action = HotspotStartupPolicy.startAction( p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, existingGroupName = "DIRECT-BC-OLDGROUP", - ownedGroupName = null + ownedGroupName = null, + confirmedGroupName = null ) - assertEquals(HotspotStartupPolicy.StartAction.RemoveStaleGroupThenCreate, action) + assertEquals(HotspotStartupPolicy.StartAction.ConfirmReplaceExisting, action) } @Test - fun `a foreign group is left alone even when we recorded nothing`() { + fun `another phone's bitchat group is not treated as ours by its prefix`() { + val action = HotspotStartupPolicy.startAction( + p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, + existingGroupName = "DIRECT-BC-THEIRS99", + ownedGroupName = "DIRECT-BC-CUF6EN63", + confirmedGroupName = null + ) + + assertEquals(HotspotStartupPolicy.StartAction.ConfirmReplaceExisting, action) + } + + @Test + fun `a foreign group needs confirmation even when we recorded nothing`() { val action = HotspotStartupPolicy.startAction( p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, existingGroupName = "DIRECT-xY-Chromecast", - ownedGroupName = null + ownedGroupName = null, + confirmedGroupName = null ) - assertEquals( - HotspotStartupPolicy.StartAction.Fail(HotspotStartupPolicy.FOREIGN_GROUP_MESSAGE), - action - ) + assertEquals(HotspotStartupPolicy.StartAction.ConfirmReplaceExisting, action) } @Test @@ -126,18 +160,35 @@ class HotspotStartupPolicyTest { val action = HotspotStartupPolicy.startAction( p2pState = WifiP2pManager.WIFI_P2P_STATE_ENABLED, existingGroupName = null, - ownedGroupName = null + ownedGroupName = null, + confirmedGroupName = null ) assertEquals(HotspotStartupPolicy.StartAction.Create, action) } + @Test + fun `confirmation does not skip the disabled-P2P check`() { + val action = HotspotStartupPolicy.startAction( + p2pState = WifiP2pManager.WIFI_P2P_STATE_DISABLED, + existingGroupName = "DIRECT-xY-Chromecast", + ownedGroupName = null, + confirmedGroupName = "DIRECT-xY-Chromecast" + ) + + assertEquals( + HotspotStartupPolicy.StartAction.Fail(HotspotStartupPolicy.P2P_DISABLED_MESSAGE), + action + ) + } + @Test fun `disabled P2P fails before touching any existing group`() { val action = HotspotStartupPolicy.startAction( p2pState = WifiP2pManager.WIFI_P2P_STATE_DISABLED, existingGroupName = "DIRECT-BC-CUF6EN63", - ownedGroupName = "DIRECT-BC-CUF6EN63" + ownedGroupName = "DIRECT-BC-CUF6EN63", + confirmedGroupName = null ) assertEquals( From 5859ac29cd894b056844d45e07d56fffb2194678 Mon Sep 17 00:00:00 2001 From: Moe Hamade <69801237+moehamade@users.noreply.github.com> Date: Thu, 30 Jul 2026 02:23:51 +0300 Subject: [PATCH 3/4] fix(hotspot): confirm a group is ours before trusting or removing it Two related ways the app could act on a group it did not create. createdGroup was set once and never revisited, so it outlived the group it described. Our group can disappear without us - Wi-Fi toggled, another app issuing its own device-scoped removeGroup(), a driver reset - and another app can create one in its place while the sharing screen is still open. Stop then believed the group present was ours and removed it, disconnecting a session the user never agreed to touch. Every group snapshot now reconciles the flag against what is actually on the framework. isGroupOwner cannot carry that judgement on its own: it reports that this DEVICE hosts the group, which is equally true of an autonomous group another app created here. So a replacement group was still being adopted - its name persisted as ownedGroupName, its credentials shown as ours below Q - and because ownership is now an exact-name match, the next start classified that name as ours and removed it with no confirmation. Snapshots are only read once the group is confirmed to be the one we created: above Q by the network name we chose, below Q by the first name the framework reported for our own successful creation, fixed from then on. Reconciliation waits until our group has been named, because a null snapshot during formation says nothing about a group that has not appeared yet. Losing the flag to a transient null is safe in a way that keeping it is not - the group we created is left behind, and the next start recognises it by name and removes it silently. A group-info reply arriving after the stop is now ignored, so it cannot revive state the stop just cleared. Device-verified on a Pixel 9a (Android 16, API 37). Co-Authored-By: Claude Opus 5 (1M context) --- .../bitchat/android/hotspot/HotspotManager.kt | 129 ++++++++++++++---- 1 file changed, 100 insertions(+), 29 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt index 5f30b8ec..466e38d9 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt @@ -63,12 +63,17 @@ class HotspotManager(private val context: Context) { private var hasNotifiedStarted = false // Track if we've notified the callback private var isReceiverRegistered = false // Track receiver registration to prevent leaks - // Set once our own createGroup command is accepted. stopHotspot() only calls - // removeGroup() when this is true: removal is device-scoped, so issuing it with - // nothing of ours on the framework could only tear down another app's session - // (Cast, Android Auto, Quick Share). + // Set once our own createGroup command is accepted, and re-checked against every + // group snapshot afterwards. stopHotspot() only calls removeGroup() when this is + // true: removal is device-scoped, so issuing it when the group on the framework + // is not ours could only tear down another app's session (Cast, Android Auto, + // Quick Share). private var createdGroup = false + // Framework-reported name of the group this session hosts, once known. Null while + // the group is still forming, when a null snapshot carries no information. + private var hostedGroupName: String? = null + // Name of the foreign group the user explicitly agreed to disconnect, or null. // Consent is per-group: a group with a different name asks again. private var confirmedReplacementName: String? = null @@ -199,6 +204,7 @@ class HotspotManager(private val context: Context) { val hadOwnGroup = createdGroup createdGroup = false + hostedGroupName = null if (staleChannel != null && hadOwnGroup) { try { @@ -557,34 +563,52 @@ class HotspotManager(private val context: Context) { try { wifiP2pManager?.requestGroupInfo(ch) { group -> - if (group != null) { - currentGroup = group + // A reply arriving after the hotspot stopped must not revive any + // state the stop just cleared. + if (channel !== ch) return@requestGroupInfo - // Update saved credentials if using system-generated ones - if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { - savedSsid = group.networkName - savedPassword = group.passphrase - } + reconcileGroupOwnership(group) - // Authoritative name straight from the framework. Only recorded - // for a group we host: in the client role the group is another - // app's, and recording its name would make the next run treat a - // foreign orphan as ours. - if (group.isGroupOwner) { - group.networkName?.let { ownedGroupName = it } - } - - // Notify callback on FIRST successful group info retrieval - if (!hasNotifiedStarted) { - hasNotifiedStarted = true - Log.d(TAG, "Group info received, notifying callback") - callback?.onHotspotStarted() - } else { - // Subsequent updates - callback?.onConnectionInfoUpdated(getConnectionInfo()) - } - } else { + if (group == null) { Log.w(TAG, "requestGroupInfo returned null group") + return@requestGroupInfo + } + + if (!isOurHostedGroup(group)) { + // Someone else's group is on the radio. Reading anything from it + // — its name, its credentials, its client count — would report + // another app's session as our hotspot, and recording its name + // would let the next start remove it without asking. + Log.w( + TAG, + "Observed group '${group.networkName}' is not the one we created" + ) + return@requestGroupInfo + } + + currentGroup = group + + // Update saved credentials if using system-generated ones + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + savedSsid = group.networkName + savedPassword = group.passphrase + } + + // Authoritative name straight from the framework, for the group we + // just confirmed is ours. + group.networkName?.let { + hostedGroupName = it + ownedGroupName = it + } + + // Notify callback on FIRST successful group info retrieval + if (!hasNotifiedStarted) { + hasNotifiedStarted = true + Log.d(TAG, "Group info received, notifying callback") + callback?.onHotspotStarted() + } else { + // Subsequent updates + callback?.onConnectionInfoUpdated(getConnectionInfo()) } } } catch (e: SecurityException) { @@ -593,6 +617,53 @@ class HotspotManager(private val context: Context) { } } + /** + * Is this snapshot the group this session created? + * + * `isGroupOwner` cannot answer that on its own: it reports that *this device* + * hosts the group, which is equally true of an autonomous group another app + * created here. Above Q we chose the network name, so it identifies our group + * exactly. Below Q the framework names it, and the first snapshot after our own + * createGroup succeeded is the only evidence available — after that the name is + * fixed, and a group answering to a different one is not ours. + */ + private fun isOurHostedGroup(group: WifiP2pGroup): Boolean { + if (!group.isGroupOwner) return false + val name = group.networkName ?: return false + + hostedGroupName?.let { return name == it } + + return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + name == savedSsid + } else { + true + } + } + + /** + * Keep [createdGroup] honest about what is actually on the framework. + * + * Our group can disappear without us — Wi-Fi toggled, another app issuing its own + * device-scoped removeGroup(), a driver reset — and another app can then create + * one in its place. Believing the group present is still ours would make stop + * remove that replacement, the exact disruption consent exists to prevent. + * + * Reconciled only once the framework has named our group: before that a null + * snapshot means the group is still forming, not that it is gone. Losing the flag + * to a transient null is safe in a way that keeping it is not — the group we + * created is then left behind, and the next start recognises it by name and + * removes it silently. + */ + private fun reconcileGroupOwnership(group: WifiP2pGroup?) { + val hosted = hostedGroupName ?: return + + val stillOurs = group != null && isOurHostedGroup(group) + if (createdGroup && !stillOurs) { + Log.w(TAG, "Group '$hosted' is no longer ours; leaving what is present alone") + } + createdGroup = stillOurs + } + /** * Acquire WakeLock and WifiLock to keep hotspot active. */ From b6ad8971a8c4d787c2477b0d17e5b70b8f9e7cb7 Mon Sep 17 00:00:00 2001 From: callebtc <93376500+callebtc@users.noreply.github.com> Date: Thu, 30 Jul 2026 03:20:48 +0200 Subject: [PATCH 4/4] fix(hotspot): revalidate ownership during teardown --- .../bitchat/android/hotspot/HotspotManager.kt | 139 ++++++++++++++---- .../android/hotspot/HotspotStartupPolicy.kt | 17 +++ .../hotspot/HotspotStartupPolicyTest.kt | 51 ++++++- 3 files changed, 180 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt index 466e38d9..a56182bc 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotManager.kt @@ -78,6 +78,12 @@ class HotspotManager(private val context: Context) { // Consent is per-group: a group with a different name asks again. private var confirmedReplacementName: String? = null + // stopHotspot() can be reached again while its group query/removal is still in + // flight. Later callers wait for that same teardown instead of releasing the + // Wi-Fi Aware lease early. + private var teardownInProgress = false + private val teardownCallbacks = mutableListOf<() -> Unit>() + // Saved credentials for reconnection private var savedSsid: String? = null private var savedPassword: String? = null @@ -191,6 +197,12 @@ class HotspotManager(private val context: Context) { fun stopHotspot(onTeardownComplete: (() -> Unit)? = null) { Log.d(TAG, "Stopping hotspot") + onTeardownComplete?.let(teardownCallbacks::add) + if (teardownInProgress) { + Log.d(TAG, "Teardown already in progress; chaining completion") + return + } + isStarting = false hasNotifiedStarted = false @@ -203,40 +215,27 @@ class HotspotManager(private val context: Context) { channel = null val hadOwnGroup = createdGroup + val expectedGroupName = hostedGroupName ?: if ( + Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q + ) { + savedSsid + } else { + null + } createdGroup = false hostedGroupName = null + var teardownAction: (() -> Unit)? = null if (staleChannel != null && hadOwnGroup) { - try { - wifiP2pManager?.removeGroup(staleChannel, object : ActionListener { - override fun onSuccess() { - Log.d(TAG, "Group removed successfully") - // Nothing of ours is left for a later run to clean up. - ownedGroupName = null - closeChannel(staleChannel) - onTeardownComplete?.invoke() - } - override fun onFailure(reason: Int) { - Log.w(TAG, "Failed to remove group: $reason") - closeChannel(staleChannel) - onTeardownComplete?.invoke() - } - }) - } catch (e: SecurityException) { - // Revoked mid-session. Closing the channel still detaches this app's - // binder, which asks the framework to drop our group with it. - Log.e(TAG, "Wi-Fi permission was revoked while removing the group", e) - closeChannel(staleChannel) - onTeardownComplete?.invoke() + teardownInProgress = true + teardownAction = { + removeOwnGroupIfStillPresent(staleChannel, expectedGroupName) } } else if (staleChannel != null) { // This session created nothing, so there is nothing of ours to remove. // removeGroup() here is exactly the bug this change fixes: device-scoped // removal would disconnect whatever group another app has running. closeChannel(staleChannel) - onTeardownComplete?.invoke() - } else { - onTeardownComplete?.invoke() } // Release locks @@ -256,6 +255,84 @@ class HotspotManager(private val context: Context) { currentGroup = null callback = null + + if (teardownAction != null) { + teardownAction.invoke() + } else { + finishTeardown() + } + } + + /** + * Re-check the device-scoped group immediately before removing it. The last poll + * is only a snapshot: our group may have disappeared and another app may have + * claimed Wi-Fi Direct before stop was requested. + */ + @SuppressLint("MissingPermission") + private fun removeOwnGroupIfStillPresent(ch: Channel, expectedGroupName: String?) { + val manager = wifiP2pManager ?: run { + closeChannel(ch) + finishTeardown() + return + } + + try { + manager.requestGroupInfo(ch) { group -> + val stillOurs = HotspotStartupPolicy.isExpectedHostedGroup( + existingGroupName = group?.networkName, + isGroupOwner = group?.isGroupOwner == true, + expectedGroupName = expectedGroupName + ) + if (!stillOurs) { + Log.i( + TAG, + "Current group '${group?.networkName}' is not ours " + + "('$expectedGroupName'); leaving it alone" + ) + closeChannel(ch) + finishTeardown() + return@requestGroupInfo + } + + try { + manager.removeGroup(ch, object : ActionListener { + override fun onSuccess() { + Log.d(TAG, "Group removed successfully") + clearOwnedGroupNameIfMatches(expectedGroupName) + closeChannel(ch) + finishTeardown() + } + + override fun onFailure(reason: Int) { + Log.w(TAG, "Failed to remove group: $reason") + closeChannel(ch) + finishTeardown() + } + }) + } catch (e: SecurityException) { + Log.e(TAG, "Wi-Fi permission was revoked while removing the group", e) + closeChannel(ch) + finishTeardown() + } + } + } catch (e: SecurityException) { + Log.e(TAG, "Wi-Fi permission was revoked while confirming group ownership", e) + closeChannel(ch) + finishTeardown() + } + } + + private fun clearOwnedGroupNameIfMatches(removedGroupName: String?) { + if (HotspotStartupPolicy.shouldClearOwnedGroupName(ownedGroupName, removedGroupName)) { + ownedGroupName = null + } + } + + private fun finishTeardown() { + teardownInProgress = false + val callbacks = teardownCallbacks.toList() + teardownCallbacks.clear() + callbacks.forEach { it.invoke() } } /** @@ -655,11 +732,21 @@ class HotspotManager(private val context: Context) { * removes it silently. */ private fun reconcileGroupOwnership(group: WifiP2pGroup?) { - val hosted = hostedGroupName ?: return + val expectedName = hostedGroupName ?: if ( + Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q + ) { + savedSsid + } else { + null + } ?: return + + // A null snapshot is normal before the configured group appears. A non-null + // group with a different name is positive evidence that ours was replaced. + if (hostedGroupName == null && group == null) return val stillOurs = group != null && isOurHostedGroup(group) if (createdGroup && !stillOurs) { - Log.w(TAG, "Group '$hosted' is no longer ours; leaving what is present alone") + Log.w(TAG, "Group '$expectedName' is no longer ours; leaving what is present alone") } createdGroup = stillOurs } diff --git a/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt b/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt index 3e9f32d8..80d6f4ee 100644 --- a/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt +++ b/app/src/main/java/com/bitchat/android/hotspot/HotspotStartupPolicy.kt @@ -77,6 +77,23 @@ internal object HotspotStartupPolicy { private fun isOurs(existingGroupName: String, ownedGroupName: String?): Boolean = ownedGroupName != null && existingGroupName == ownedGroupName + /** Only an exact owner-role name match authorizes device-scoped removal. */ + fun isExpectedHostedGroup( + existingGroupName: String?, + isGroupOwner: Boolean, + expectedGroupName: String? + ): Boolean = + isGroupOwner && + expectedGroupName != null && + existingGroupName == expectedGroupName + + /** A stale teardown must not erase the ownership marker of a newer session. */ + fun shouldClearOwnedGroupName( + storedGroupName: String?, + removedGroupName: String? + ): Boolean = + removedGroupName != null && storedGroupName == removedGroupName + /** * @param reason a [WifiP2pManager] failure reason from `ActionListener.onFailure` * @param attempt 1-based attempt that just failed diff --git a/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt b/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt index a33577a3..4cc68492 100644 --- a/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/hotspot/HotspotStartupPolicyTest.kt @@ -2,6 +2,7 @@ package com.bitchat.android.hotspot import android.net.wifi.p2p.WifiP2pManager import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test @@ -218,4 +219,52 @@ class HotspotStartupPolicyTest { assertTrue(decision is HotspotStartupPolicy.Decision.Fail) } -} \ No newline at end of file + + @Test + fun `only the expected hosted group may be removed on stop`() { + assertTrue( + HotspotStartupPolicy.isExpectedHostedGroup( + existingGroupName = "DIRECT-BC-OURS1234", + isGroupOwner = true, + expectedGroupName = "DIRECT-BC-OURS1234" + ) + ) + assertFalse( + HotspotStartupPolicy.isExpectedHostedGroup( + existingGroupName = "DIRECT-xY-Cast", + isGroupOwner = true, + expectedGroupName = "DIRECT-BC-OURS1234" + ) + ) + assertFalse( + HotspotStartupPolicy.isExpectedHostedGroup( + existingGroupName = "DIRECT-BC-OURS1234", + isGroupOwner = false, + expectedGroupName = "DIRECT-BC-OURS1234" + ) + ) + assertFalse( + HotspotStartupPolicy.isExpectedHostedGroup( + existingGroupName = "DIRECT-BC-OURS1234", + isGroupOwner = true, + expectedGroupName = null + ) + ) + } + + @Test + fun `an old teardown cannot clear a newer ownership marker`() { + assertTrue( + HotspotStartupPolicy.shouldClearOwnedGroupName( + storedGroupName = "DIRECT-BC-OLD12345", + removedGroupName = "DIRECT-BC-OLD12345" + ) + ) + assertFalse( + HotspotStartupPolicy.shouldClearOwnedGroupName( + storedGroupName = "DIRECT-BC-NEW67890", + removedGroupName = "DIRECT-BC-OLD12345" + ) + ) + } +}