diff --git a/buildSrc/src/main/kotlin/ProjectConfig.kt b/buildSrc/src/main/kotlin/ProjectConfig.kt index e7ae39d83..8386cec82 100644 --- a/buildSrc/src/main/kotlin/ProjectConfig.kt +++ b/buildSrc/src/main/kotlin/ProjectConfig.kt @@ -55,6 +55,6 @@ const val NEWPIPE_APPLICATION_ID_NEW = "net.newpipe.app" // vc=19 / 0.1.0-AE — rust pipeline cutover. Extraction via // strawcore-core (Sulkta-Coop/strawcore) via the UniFFI wrapper; no // NewPipeExtractor in the runtime path. -const val STRAW_VERSION_CODE = 40 -const val STRAW_VERSION_NAME = "0.1.0-AZ" +const val STRAW_VERSION_CODE = 41 +const val STRAW_VERSION_NAME = "0.1.0-BA" const val STRAW_APPLICATION_ID = "com.sulkta.straw" diff --git a/rust/strawcore/src/runtime.rs b/rust/strawcore/src/runtime.rs index bc02f3879..7e1ef14c8 100644 --- a/rust/strawcore/src/runtime.rs +++ b/rust/strawcore/src/runtime.rs @@ -43,32 +43,33 @@ fn now_ms() -> u64 { } pub fn ensure_initialized() { - // Fast path: already initialized. Just an Acquire load. + // Fast path: already initialized. Single Acquire load. if INITIALIZED.load(Ordering::Acquire) { return; } - // Acquire the lock FIRST. The mutex is the in-progress queue — - // concurrent callers wait here while one thread does init. - // Round-5 audit HIGH-1: the prior shape stamped LAST_ATTEMPT_MS - // at the start of an attempt then let concurrent callers - // short-circuit-out via the backoff check; they'd then call into - // the extractor before init completed → DownloaderMissing. - let _guard = match INIT_LOCK.lock() { - Ok(g) => g, - Err(p) => p.into_inner(), - }; - // Re-check under the lock — another thread may have just succeeded. - if INITIALIZED.load(Ordering::Acquire) { - return; - } - // Now the backoff check — but ONLY skips when a *prior* failed - // attempt is still in cooldown. If LAST_ATTEMPT_MS is zero, no - // attempt has been made yet; the lock fall-through proceeds. + // Backoff check BEFORE the lock — a recent failure shouldn't + // make N concurrent callers queue on a mutex they'll all skip + // out of anyway. let last = LAST_ATTEMPT_MS.load(Ordering::Acquire); let now = now_ms(); if last != 0 && now.saturating_sub(last) < RETRY_BACKOFF_MS { return; } + // try_lock — if another thread is already mid-init, return + // immediately rather than block. The caller will get + // DownloaderMissing once from the extractor and recover on + // the next user action; the alternative (blocking N tokio + // workers for the full duration of a slow init) freezes the + // UI. Round-6 audit HIGH-2 was the regression on round-5's + // mutex-first ordering. + let _guard = match INIT_LOCK.try_lock() { + Ok(g) => g, + Err(_) => return, + }; + // Re-check under the lock — another thread may have just succeeded. + if INITIALIZED.load(Ordering::Acquire) { + return; + } match ReqwestDownloader::new() { Ok(dl) => { NewPipe::init_full( diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt index c32b92320..4860ef21c 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt @@ -47,8 +47,12 @@ class ChannelViewModel : ViewModel() { // Round-5 audit MED-3: extractor-emitted uploaderUrl can be // attacker-controlled if the YT response is poisoned upstream. // Refuse non-YT hosts at the entry point so we don't even - // issue a network call to evil.com via strawcore. + // issue a network call to evil.com via strawcore. Round-6 + // audit HIGH-1: also cancel inFlight on rejection so a + // still-resolving prior load can't clobber the error banner. if (!isAllowedYtUrl(channelUrl)) { + inFlight?.cancel() + inFlight = null _ui.update { ChannelUiState(loading = false, error = "Unsupported URL") } return } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt index a54be30a7..ed0c0e044 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt @@ -110,7 +110,13 @@ object SettingsImport { suspend fun run(context: Context, zipUri: Uri): Result = withContext(Dispatchers.IO) { - runCatching { runInner(context, zipUri) } + // runInner is suspend (it switches to NonCancellable for + // cleanup). Plain runCatching would swallow a user-back + // CancellationException and surface it as a normal + // failure with a misleading banner. Round-6 audit HIGH-2. + com.sulkta.straw.util.runCatchingCancellable { + runInner(context, zipUri) + } } /** @@ -194,6 +200,14 @@ object SettingsImport { } when (entry.name) { "newpipe.db" -> { + // Reject duplicate entries — a malicious zip + // can put a benign db first and a hostile + // second; ZipInputStream walks in order and + // would overwrite. Round-6 audit MED-5. + if (dbFile != null) { + warnings += "duplicate newpipe.db in archive — aborting" + return null to null + } val out = File(workDir, "newpipe.db") val written = copyBounded(zip, out, MAX_DB_BYTES) if (written < 0L) { @@ -204,6 +218,10 @@ object SettingsImport { dbFile = out } "preferences.json" -> { + if (prefs != null) { + warnings += "duplicate preferences.json in archive — aborting" + return null to null + } val bytes = readBoundedBytes(zip, MAX_PREFS_BYTES) if (bytes == null) { warnings += "preferences.json exceeds ${MAX_PREFS_BYTES / 1024} KB — skipping" @@ -303,7 +321,10 @@ object SettingsImport { var itemsAdded = 0 openDb(dbFile).use { db -> val playlistRows = mutableListOf>() - db.rawQuery("SELECT uid, name FROM playlists", null).use { c -> + // Hard caps so a malicious export with millions of rows + // doesn't walk an unbounded cursor into memory. Round-6 + // audit MED-3. + db.rawQuery("SELECT uid, name FROM playlists LIMIT 256", null).use { c -> while (c.moveToNext()) { val uid = c.getLong(0) val name = c.getString(1) ?: "Untitled" @@ -319,6 +340,7 @@ object SettingsImport { JOIN streams s ON s.uid = j.stream_id WHERE j.playlist_id = ? ORDER BY j.join_index + LIMIT 5000 """.trimIndent(), arrayOf(uid.toString()), ).use { c -> @@ -470,8 +492,15 @@ object SettingsImport { for ((key, cat) in targets) { val want = prefs.boolOrNull(key) ?: continue val have = cat in current - if (want != have) settings.toggle(cat) - applied++ + // Only count an applied toggle when it actually + // changed something. Prior shape counted every + // observed key, inflating the import summary to + // "12 settings applied" when only 2 changed. + // Round-6 audit MED-2. + if (want != have) { + settings.toggle(cat) + applied++ + } } } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt index 9918c0ead..33b81c305 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt @@ -103,8 +103,14 @@ class VideoDetailViewModel : ViewModel() { if (loadedUrl == streamUrl && _ui.value.detail != null) return // Same YT-host gate as ChannelViewModel — covers the case // where a tap on a poisoned related-card lands here. - // Round-5 audit MED-3. + // Round-5 audit MED-3. Round-6 audit HIGH-1: cancel any + // in-flight load on rejection too — otherwise the + // late-arriving prior-job's fence still PASSES (loadedUrl + // wasn't moved) and clobbers the "Unsupported URL" error + // banner. if (!isAllowedYtUrl(streamUrl)) { + inFlight?.cancel() + inFlight = null _ui.update { VideoDetailUiState(loading = false, error = "Unsupported URL") } return } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt index a2062e7f0..4d455a47a 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt @@ -402,5 +402,8 @@ private fun pickActiveSegment( ): SbSegment? = segments.firstOrNull { s -> val uuidNotSkipped = s.UUID == null || s.UUID !in skipped val interval = s.endSec - s.startSec > 0.1 - uuidNotSkipped && interval && posSec >= s.startSec && posSec < s.endSec - 0.05 + // Drop the prior -0.05 exclusion — combined with the loop's + // 150ms polling cadence, short SB segments could fall in the + // gap and silently skip the skip. Round-6 audit MED-4. + uuidNotSkipped && interval && posSec >= s.startSec && posSec < s.endSec } diff --git a/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt b/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt index 8ab53bf01..2e64950fe 100644 --- a/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt +++ b/strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt @@ -14,6 +14,7 @@ import com.sulkta.straw.data.Settings import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -96,6 +97,13 @@ class SearchViewModel : ViewModel() { // previous network call rather than racing it. Round-4 audit // HIGH-2: `_ui.value = _ui.value.copy()` patterns + concurrent // submits were both lost-write hazards. + // + // Fence by Job identity, not query string. Round-6 audit HIGH-1: + // `onQueryChange` mutates _ui.value.query for reactive filtering + // WITHOUT cancelling inFlight, so a string-equality fence treats + // a still-valid result as stale just because the user kept typing + // after submit. Job identity captures the "I am the active + // submit" intent — only a fresh submit cancels me. private var inFlight: Job? = null fun onQueryChange(q: String) { @@ -174,14 +182,13 @@ class SearchViewModel : ViewModel() { uploadDateRelative = r.uploadDateRelative, ) } - // Fence ALL post-network side-effects against a - // fresher submit that cancelled this one — not just - // the UI update. Round-5 audit HIGH-2: the prior - // fence only guarded `_ui.update`, so the cache write - // + pool rebuild proceeded for a cancelled query, - // polluting SearchCache with results the user - // abandoned mid-stroke. - if (_ui.value.query.trim() != q) return@launch + // Fence by job identity (ensureActive) — only a fresh + // submit that called inFlight?.cancel() invalidates + // me. Bare typing in the search bar (onQueryChange) + // doesn't cancel anything, so our results are still + // valid even if `_ui.value.query` moved on. + // Round-6 audit HIGH-1. + ensureActive() _ui.update { it.copy( loading = false, @@ -194,10 +201,9 @@ class SearchViewModel : ViewModel() { runCatching { History.get().recordSearch(q) } if (Settings.get().cacheEnabled.value) { withContext(Dispatchers.IO) { - // Re-check after suspending — a fresh submit - // can have changed the query between the UI - // update and now. - if (_ui.value.query.trim() != q) return@withContext + // Re-check active state after the dispatcher + // switch (still cooperative cancellation). + ensureActive() runCatching { SearchCache.get().record(q, items) } // Refresh the in-memory pool with the new // entries so subsequent reactive filters see @@ -207,7 +213,6 @@ class SearchViewModel : ViewModel() { } } catch (t: Throwable) { if (t is CancellationException) throw t - if (_ui.value.query.trim() != q) return@launch // Keep the cached preview visible on network failure so // the user still has something to look at while offline. _ui.update {