From e48f9aa8289ea492fac8b14f63f880f27a3dcac0 Mon Sep 17 00:00:00 2001 From: Peter Cai Date: Fri, 22 Mar 2024 21:08:59 -0400 Subject: [PATCH] refactor: Channel validity, and reconnection * ApduInterfaces also need a concept of validity based on the underlying APDU channel. For example, OMAPI depends on SEService being still connected. * We then rely on this validity to wait for reconnection; we do not need to manually remove all channels under a slot because the rest will be invalid anyway, and the next attempt at connection will lazily recreate the channel. * We had to manage channels manually before during reconnect because `valid` may result in SIGSEGV's when the underlying APDU channel has become invalid. This is avoided by the validity concept added to APDU channels. --- .../core/DefaultEuiccChannelManager.kt | 41 +++++++++---------- .../openeuicc/core/EuiccChannelManager.kt | 11 +++-- .../openeuicc/core/OmapiApduInterface.kt | 3 ++ .../openeuicc/ui/EuiccManagementFragment.kt | 2 +- .../core/TelephonyManagerApduInterface.kt | 5 +++ .../net/typeblog/lpac_jni/ApduInterface.kt | 7 ++++ .../lpac_jni/LocalProfileAssistant.kt | 9 ---- .../impl/LocalProfileAssistantImpl.kt | 22 ++++++++-- 8 files changed, 60 insertions(+), 40 deletions(-) diff --git a/app-common/src/main/java/im/angry/openeuicc/core/DefaultEuiccChannelManager.kt b/app-common/src/main/java/im/angry/openeuicc/core/DefaultEuiccChannelManager.kt index ec676d1..811c56c 100644 --- a/app-common/src/main/java/im/angry/openeuicc/core/DefaultEuiccChannelManager.kt +++ b/app-common/src/main/java/im/angry/openeuicc/core/DefaultEuiccChannelManager.kt @@ -103,28 +103,35 @@ open class DefaultEuiccChannelManager( findAllEuiccChannelsByPhysicalSlot(physicalSlotId) } - override fun findEuiccChannelByPortBlocking(physicalSlotId: Int, portId: Int): EuiccChannel? = - runBlocking { - withContext(Dispatchers.IO) { - uiccCards.find { it.physicalSlotIndex == physicalSlotId }?.let { card -> - card.ports.find { it.portIndex == portId }?.let { tryOpenEuiccChannel(it) } - } + override suspend fun findEuiccChannelByPort(physicalSlotId: Int, portId: Int): EuiccChannel? = + withContext(Dispatchers.IO) { + uiccCards.find { it.physicalSlotIndex == physicalSlotId }?.let { card -> + card.ports.find { it.portIndex == portId }?.let { tryOpenEuiccChannel(it) } } } - override suspend fun tryReconnectSlot(physicalSlotId: Int, timeoutMillis: Long) { - invalidateByPhysicalSlot(physicalSlotId) + override fun findEuiccChannelByPortBlocking(physicalSlotId: Int, portId: Int): EuiccChannel? = + runBlocking { + findEuiccChannelByPort(physicalSlotId, portId) + } + + override suspend fun waitForReconnect(physicalSlotId: Int, portId: Int, timeoutMillis: Long) { + // If there is already a valid channel, we close it proactively + // Sometimes the current channel can linger on for a bit even after it should have become invalid + channels.find { it.slotId == physicalSlotId && it.portId == portId }?.apply { + if (valid) close() + } withTimeout(timeoutMillis) { while (true) { try { - findAllEuiccChannelsByPhysicalSlot(physicalSlotId)!!.forEach { - check(it.valid) { "Invalid channel" } - } + // tryOpenEuiccChannel() will automatically dispose of invalid channels + // and recreate when needed + val channel = findEuiccChannelByPortBlocking(physicalSlotId, portId)!! + check(channel.valid) { "Invalid channel" } break } catch (e: Exception) { - Log.d(TAG, "Slot reconnect failure, retrying in 1000 ms") - invalidateByPhysicalSlot(physicalSlotId) + Log.d(TAG, "Slot $physicalSlotId port $portId reconnect failure, retrying in 1000 ms") } delay(1000) } @@ -157,12 +164,4 @@ open class DefaultEuiccChannelManager( channels.clear() euiccChannelFactory.cleanup() } - - private fun invalidateByPhysicalSlot(physicalSlotId: Int) { - val toRemove = channels.filter { it.valid && it.slotId == physicalSlotId } - for (channel in toRemove) { - channel.close() - channels.remove(channel) - } - } } \ No newline at end of file diff --git a/app-common/src/main/java/im/angry/openeuicc/core/EuiccChannelManager.kt b/app-common/src/main/java/im/angry/openeuicc/core/EuiccChannelManager.kt index 5ee40f2..9ec3030 100644 --- a/app-common/src/main/java/im/angry/openeuicc/core/EuiccChannelManager.kt +++ b/app-common/src/main/java/im/angry/openeuicc/core/EuiccChannelManager.kt @@ -9,13 +9,11 @@ interface EuiccChannelManager { suspend fun enumerateEuiccChannels() /** - * Reconnect ALL EuiccChannels belonging to a physical slot - * Throws TimeoutCancellationException when timed out - * If this operation times out, none of the channels belonging to the slot will be - * guaranteed to be consistent. The caller should either call invalidate() - * and try again later, or the application should simply exit entirely. + * Wait for a slot + port to reconnect (i.e. become valid again) + * If the port is currently valid, this function will return immediately. + * On timeout, the caller can decide to either try again later, or alert the user with an error */ - suspend fun tryReconnectSlot(physicalSlotId: Int, timeoutMillis: Long = 1000) + suspend fun waitForReconnect(physicalSlotId: Int, portId: Int, timeoutMillis: Long = 1000) /** * Returns the EuiccChannel corresponding to a **logical** slot @@ -39,6 +37,7 @@ interface EuiccChannelManager { /** * Returns the EuiccChannel corresponding to a **physical** slot and a port ID */ + suspend fun findEuiccChannelByPort(physicalSlotId: Int, portId: Int): EuiccChannel? fun findEuiccChannelByPortBlocking(physicalSlotId: Int, portId: Int): EuiccChannel? /** diff --git a/app-common/src/main/java/im/angry/openeuicc/core/OmapiApduInterface.kt b/app-common/src/main/java/im/angry/openeuicc/core/OmapiApduInterface.kt index 3d3bfe1..227977c 100644 --- a/app-common/src/main/java/im/angry/openeuicc/core/OmapiApduInterface.kt +++ b/app-common/src/main/java/im/angry/openeuicc/core/OmapiApduInterface.kt @@ -13,6 +13,9 @@ class OmapiApduInterface( private lateinit var session: Session private lateinit var lastChannel: Channel + override val valid: Boolean + get() = service.isConnected && (this::session.isInitialized && !session.isClosed) + override fun connect() { session = service.getUiccReaderCompat(port.logicalSlotIndex + 1).openSession() } diff --git a/app-common/src/main/java/im/angry/openeuicc/ui/EuiccManagementFragment.kt b/app-common/src/main/java/im/angry/openeuicc/ui/EuiccManagementFragment.kt index c02f7fd..c1e5ba6 100644 --- a/app-common/src/main/java/im/angry/openeuicc/ui/EuiccManagementFragment.kt +++ b/app-common/src/main/java/im/angry/openeuicc/ui/EuiccManagementFragment.kt @@ -146,7 +146,7 @@ open class EuiccManagementFragment : Fragment(), EuiccProfilesChangedListener, } try { - euiccChannelManager.tryReconnectSlot(slotId, timeoutMillis = 30 * 1000) + euiccChannelManager.waitForReconnect(slotId, portId, timeoutMillis = 30 * 1000) } catch (e: TimeoutCancellationException) { withContext(Dispatchers.Main) { // Timed out waiting for SIM to come back online, we can no longer assume that the LPA is still valid diff --git a/app/src/main/java/im/angry/openeuicc/core/TelephonyManagerApduInterface.kt b/app/src/main/java/im/angry/openeuicc/core/TelephonyManagerApduInterface.kt index d6770c3..509d7ee 100644 --- a/app/src/main/java/im/angry/openeuicc/core/TelephonyManagerApduInterface.kt +++ b/app/src/main/java/im/angry/openeuicc/core/TelephonyManagerApduInterface.kt @@ -11,6 +11,11 @@ class TelephonyManagerApduInterface( ): ApduInterface { private var lastChannel: Int = -1 + override val valid: Boolean + // TelephonyManager channels will never become truly "invalid", + // just that transactions might return errors or nonsense + get() = lastChannel != -1 + override fun connect() { // Do nothing } diff --git a/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/ApduInterface.kt b/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/ApduInterface.kt index 44d1a3e..dfa92df 100644 --- a/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/ApduInterface.kt +++ b/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/ApduInterface.kt @@ -9,4 +9,11 @@ interface ApduInterface { fun logicalChannelOpen(aid: ByteArray): Int fun logicalChannelClose(handle: Int) fun transmit(tx: ByteArray): ByteArray + + /** + * Is this APDU connection still valid? + * Note that even if this returns true, the underlying connection might be broken anyway; + * callers should further check with the LPA to fully determine the validity of a channel + */ + val valid: Boolean } \ No newline at end of file diff --git a/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/LocalProfileAssistant.kt b/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/LocalProfileAssistant.kt index 118a963..16a91e7 100644 --- a/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/LocalProfileAssistant.kt +++ b/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/LocalProfileAssistant.kt @@ -2,15 +2,6 @@ package net.typeblog.lpac_jni interface LocalProfileAssistant { val valid: Boolean - get() = try { - // If we can read both eID and profiles properly, we are likely looking at - // a valid LocalProfileAssistant - eID - profiles - true - } catch (e: Exception) { - false - } val profiles: List val notifications: List val eID: String diff --git a/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/impl/LocalProfileAssistantImpl.kt b/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/impl/LocalProfileAssistantImpl.kt index 98a3969..a76ba75 100644 --- a/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/impl/LocalProfileAssistantImpl.kt +++ b/libs/lpac-jni/src/main/java/net/typeblog/lpac_jni/impl/LocalProfileAssistantImpl.kt @@ -11,13 +11,14 @@ import net.typeblog.lpac_jni.LocalProfileNotification import net.typeblog.lpac_jni.ProfileDownloadCallback class LocalProfileAssistantImpl( - apduInterface: ApduInterface, + private val apduInterface: ApduInterface, httpInterface: HttpInterface ): LocalProfileAssistant { companion object { private const val TAG = "LocalProfileAssistantImpl" } + private var finalized = false private var contextHandle: Long = LpacJni.createContext(apduInterface, httpInterface) init { if (LpacJni.euiccInit(contextHandle) < 0) { @@ -28,6 +29,17 @@ class LocalProfileAssistantImpl( httpInterface.usePublicKeyIds(pkids) } + override val valid: Boolean + get() = !finalized && apduInterface.valid && try { + // If we can read both eID and profiles properly, we are likely looking at + // a valid LocalProfileAssistant + eID + profiles + true + } catch (e: Exception) { + false + } + override val profiles: List get() = LpacJni.es10cGetProfilesInfo(contextHandle)!!.asList() @@ -71,8 +83,12 @@ class LocalProfileAssistantImpl( return LpacJni.es10cSetNickname(contextHandle, iccid, nickname) == 0 } + @Synchronized override fun close() { - LpacJni.euiccFini(contextHandle) - LpacJni.destroyContext(contextHandle) + if (!finalized) { + LpacJni.euiccFini(contextHandle) + LpacJni.destroyContext(contextHandle) + finalized = true + } } } \ No newline at end of file