From df8f4f5d7bb593ae68b5bcc9585c40b02ebec1d6 Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Sun, 6 Sep 2026 14:04:04 +0700 Subject: [PATCH] fix(android): scope SAF read failures to each descriptor Remove the activity-wide descriptor failure flag and centralize direct-read/fallback handling. A pipe, revoked URI, or malformed file no longer disables direct metadata reads for subsequent files. Test fallback isolation, descriptor cleanup, and a successful direct read after a failure. --- .../kotlin/com/zarz/spotiflac/MainActivity.kt | 4 - .../com/zarz/spotiflac/MainActivitySafIo.kt | 132 ++++++------------ .../com/zarz/spotiflac/MainActivitySafScan.kt | 5 - .../zarz/spotiflac/SafMetadataReadPolicy.kt | 12 ++ .../spotiflac/SafMetadataReadPolicyTest.kt | 42 ++++++ 5 files changed, 94 insertions(+), 101 deletions(-) create mode 100644 android/app/src/main/kotlin/com/zarz/spotiflac/SafMetadataReadPolicy.kt create mode 100644 android/app/src/test/kotlin/com/zarz/spotiflac/SafMetadataReadPolicyTest.kt diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt index 50f5e721..b3b82a6f 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivity.kt @@ -93,10 +93,6 @@ class MainActivity: FlutterFragmentActivity() { private val playbackLeases = LinkedHashMap() @Volatile internal var safScanCancel = false @Volatile internal var safScanActive = false - /** Tri-state: null = untested, true = works, false = fails (Samsung SELinux). */ - @Volatile internal var procSelfFdReadable: Boolean? = null - @Volatile internal var procSelfFdFallbacks: Int = 0 - internal val procSelfFdStateLock = Any() private val safTreeLauncher = registerForActivityResult( ActivityResultContracts.StartActivityForResult() ) { activityResult -> diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafIo.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafIo.kt index 2546feeb..727644a4 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafIo.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafIo.kt @@ -258,100 +258,48 @@ private fun isSeekableSafDescriptor(descriptor: ParcelFileDescriptor): Boolean { } } -internal fun MainActivity.readAudioMetadataFromUri( - uri: Uri, - displayNameHint: String? = null, - fallbackExt: String? = null, - coverCacheKey: String = "", - ): JSONObject? { - val displayName = buildUriDisplayName(uri, displayNameHint, fallbackExt) - - // Skip /proc/self/fd/ attempt when known to fail (e.g. Samsung SELinux). - if (procSelfFdReadable != false) { - try { - contentResolver.openFileDescriptor(uri, "r")?.use { pfd -> - if (!isSeekableSafDescriptor(pfd)) { - synchronized(procSelfFdStateLock) { - procSelfFdReadable = false - procSelfFdFallbacks = 0 - } - return@use - } - val directPath = "/proc/self/fd/${pfd.fd}" - val metadataJson = Gobackend.readAudioMetadataWithHintAndCoverCacheKeyJSON( - directPath, - displayName, - coverCacheKey, - ) - if (metadataJson.isNotBlank()) { - val obj = JSONObject(metadataJson) - val filenameFallback = obj.optBoolean("metadataFromFilename", false) - if (!obj.has("error") && !filenameFallback) { - synchronized(procSelfFdStateLock) { - procSelfFdReadable = true - procSelfFdFallbacks = 0 - } - return obj - } - // One filename fallback should not disable descriptors for all files. - synchronized(procSelfFdStateLock) { - procSelfFdFallbacks++ - if (procSelfFdFallbacks >= 3 && procSelfFdReadable == null) { - procSelfFdReadable = false - android.util.Log.d( - "SpotiFLAC", - "Direct /proc/self/fd read not usable for this provider, " + - "using temp-file fallback", - ) - } - } - } - } - } catch (e: Exception) { - synchronized(procSelfFdStateLock) { - if (procSelfFdReadable == null) { - procSelfFdReadable = false - android.util.Log.d( - "SpotiFLAC", - "Direct /proc/self/fd read not usable on this device, " + - "using temp-file fallback for remaining files", - ) - } - } +/** Read-only metadata uses a seekable descriptor first. Capability belongs to + * this descriptor: a pipe or revoked URI must not disable other providers. */ +private fun MainActivity.readMetadataFromUri( + uri: Uri, + displayNameHint: String? = null, + fallbackExt: String? = null, + acceptDirect: (JSONObject) -> Boolean = { true }, + read: (String, String) -> JSONObject?, +): JSONObject? { + val displayName = buildUriDisplayName(uri, displayNameHint, fallbackExt) + return readSafMetadataWithFallback( + directRead = { + contentResolver.openFileDescriptor(uri, "r")?.use { descriptor -> + if (!isSeekableSafDescriptor(descriptor)) return@use null + read("/proc/self/fd/${descriptor.fd}", displayName)?.takeIf(acceptDirect) } - } + }, + fallbackRead = { + val tempPath = copyUriToTemp(uri, fallbackExt) + if (tempPath == null) null else try { + read(tempPath, displayName) + } finally { + try { File(tempPath).delete() } catch (_: Exception) {} + } + }, + ) +} - val tempPath = try { - copyUriToTemp(uri, fallbackExt) - } catch (e: Exception) { - android.util.Log.w( - "SpotiFLAC", - "SAF metadata fallback copy failed for $uri: ${e.message}", - ) - null - } ?: return null - - try { - val metadataJson = Gobackend.readAudioMetadataWithHintAndCoverCacheKeyJSON( - tempPath, - displayName, - coverCacheKey, - ) - if (metadataJson.isBlank()) return null - val obj = JSONObject(metadataJson) - return if (obj.has("error")) null else obj - } catch (e: Exception) { - android.util.Log.w( - "SpotiFLAC", - "SAF metadata temp read failed for $uri: ${e.message}", - ) - return null - } finally { - try { - File(tempPath).delete() - } catch (_: Exception) {} - } - } +internal fun MainActivity.readAudioMetadataFromUri( + uri: Uri, + displayNameHint: String? = null, + fallbackExt: String? = null, + coverCacheKey: String = "", +): JSONObject? = readMetadataFromUri( + uri, displayNameHint, fallbackExt, + acceptDirect = { !it.optBoolean("metadataFromFilename", false) }, +) { path, name -> + val obj = JSONObject(Gobackend.readAudioMetadataWithHintAndCoverCacheKeyJSON( + path, name, coverCacheKey, + )) + obj.takeUnless { it.has("error") } +} internal fun MainActivity.writeUriFromPath(uri: Uri, srcPath: String): Boolean { val srcFile = File(srcPath) diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafScan.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafScan.kt index d3bbf9d8..dd62dd2f 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafScan.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/MainActivitySafScan.kt @@ -57,11 +57,6 @@ internal fun MainActivity.resetSafScanProgress() { synchronized(safScanLock) { safScanProgress = MainActivity.SafScanProgress() } - // Allow re-probing /proc/self/fd readability on every new scan session. - synchronized(procSelfFdStateLock) { - procSelfFdReadable = null - procSelfFdFallbacks = 0 - } } internal fun MainActivity.updateSafScanProgress(block: (MainActivity.SafScanProgress) -> Unit) { diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/SafMetadataReadPolicy.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/SafMetadataReadPolicy.kt new file mode 100644 index 00000000..5c2f3a7d --- /dev/null +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/SafMetadataReadPolicy.kt @@ -0,0 +1,12 @@ +package com.zarz.spotiflac + +/** Failure belongs to this read, never to a different document or provider. + * The direct callback owns and closes its descriptor before fallback starts. */ +internal fun readSafMetadataWithFallback( + directRead: () -> T?, + fallbackRead: () -> T?, +): T? { + val direct = try { directRead() } catch (_: Exception) { null } + if (direct != null) return direct + return try { fallbackRead() } catch (_: Exception) { null } +} diff --git a/android/app/src/test/kotlin/com/zarz/spotiflac/SafMetadataReadPolicyTest.kt b/android/app/src/test/kotlin/com/zarz/spotiflac/SafMetadataReadPolicyTest.kt new file mode 100644 index 00000000..95e88052 --- /dev/null +++ b/android/app/src/test/kotlin/com/zarz/spotiflac/SafMetadataReadPolicyTest.kt @@ -0,0 +1,42 @@ +package com.zarz.spotiflac + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Assert.assertNull +import org.junit.Test + +class SafMetadataReadPolicyTest { + @Test fun fallbackFailureIsIsolatedToOneFile() { + assertNull(readSafMetadataWithFallback({ null }, { throw IllegalArgumentException("malformed metadata") })) + assertEquals("next file", readSafMetadataWithFallback({ "next file" }, { error("copy") })) + } + + @Test fun seekableReadAvoidsTempCopy() { + val metadata = mapOf("lyrics" to "words", "replaygain_track_gain" to "-6 dB") + assertEquals(metadata, readSafMetadataWithFallback({ metadata }, { error("Unexpected copy") })) + } + + @Test fun pipeAndRevokedUriDoNotDisableNextDescriptor() { + var directReads = 0 + for (fails in listOf(true, false)) { + var closed = false + val value = readSafMetadataWithFallback( + directRead = { + directReads++ + try { + if (fails) throw SecurityException("revoked URI") + "descriptor metadata" + } finally { closed = true } + }, + fallbackRead = { + assertTrue(closed) + "temporary metadata" + }, + ) + assertEquals(if (fails) "temporary metadata" else "descriptor metadata", value) + } + assertEquals(2, directReads) + assertEquals("pipe fallback", readSafMetadataWithFallback({ null }, { "pipe fallback" })) + assertEquals("next provider", readSafMetadataWithFallback({ "next provider" }, { error("copy") })) + } +}