Sulkta-Coop/straw
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

vc=41: loop round 3/5 — round-2 misses caught + duplicate-entry zip guard

Browse source 
Three Opus round-6 audits found important regressions on the vc=39/40
fixes plus a fresh hostile-zip attack surface.

HIGH
  R6-1  SearchViewModel.submit fence-by-query swallowed valid
        results. `onQueryChange` mutates _ui.value.query without
        cancelling inFlight (for reactive cache filtering as the
        user types). The vc=40 string-equality fence treated a
        still-valid result as stale just because the user kept
        typing after submitting. Re-fenced by Job identity via
        ensureActive() — only a fresh submit (which calls
        inFlight?.cancel()) invalidates us, mere typing doesn't.
  R6-2  SettingsImport.run wrapped `runInner` (suspend) in plain
        runCatching, swallowing CancellationException and
        surfacing it as a Result.failure that produced a misleading
        "import failed" banner on a user-back abort. Migrated to
        runCatchingCancellable — exactly what round 5 added the
        util for; this call site was missed.
  R6-3  VideoDetail/ChannelViewModel.load early-return on bad URL
        didn't cancel inFlight. An in-flight prior load could
        resolve past the suspension point, see its fence pass
        (loadedUrl unchanged because we didn't update it on the
        rejected call), and clobber the "Unsupported URL" error
        banner the user is looking at. Now: inFlight?.cancel() +
        inFlight = null before the early return.
  R6-4  Rust ensure_initialized mutex contention pathology. The
        vc=40 mutex-first ordering correctly serialized init but
        blocked N concurrent tokio workers for the full duration
        of a slow ReqwestDownloader::new() (e.g. ~7s on a 6s DNS
        timeout). On a 4-core phone that froze the app for 7s.
        New: backoff check FIRST (lock-free), then try_lock — if
        someone else is initializing, return immediately. Caller
        gets one DownloaderMissing they can retry past, instead
        of multi-second UI lockup.

MED
  R6-5  Hostile zip with duplicate `newpipe.db` entries could
        masquerade a benign first DB past any pre-validation and
        ship the second malicious one (ZipInputStream walks in
        order; second write overwrites). Now: reject the archive
        when either `newpipe.db` or `preferences.json` appears
        twice.
  R6-6  importPlaylists outer + inner queries had no LIMIT — a
        crafted DB with 10M playlist rows or 10M items could walk
        unbounded cursors into memory even under the 256 MB DB
        size cap. LIMIT 256 / LIMIT 5000 now match the discipline
        on the other import paths.
  R6-7  SponsorBlock pickActiveSegment had a `posSec < endSec -
        0.05` exclusion that combined with the 150ms polling
        cadence missed short (<200ms) filler/reminder segments
        entirely. Dropped — the rate-limit work made it
        unnecessary.
  R6-8  SettingsImport.importSettings `applied++` was outside the
        `want != have` branch — inflated the import-summary count
        to "12 settings applied" when only 2 changed.

Deferred:
  - PlaylistsStore URL canonicalization (still deferred — needs
    shared YT-id-extract util, not blocking)
  - SQLite header magic validation (low-value defense-in-depth)
  - SP write reorder serialization (bigger refactor)
  - updateAvatar coalesced persists (12 parallel = wasted CPU
    but not visible)
  - Proguard rules staging (paired with R8 enable, both deferred)
  - DASH/HLS maxResolution cap via TrackSelectionParameters
    (real bug but needs careful Media3 wiring)
This commit is contained in:
Kayos 2026-05-25 15:25:25 -07:00
parent da48109a4d
commit ecc54aaf38
7 changed files with 88 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

4
buildSrc/src/main/kotlin/ProjectConfig.kt
View file

@ -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"

37
rust/strawcore/src/runtime.rs
View file

@ -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(

6
strawApp/src/main/kotlin/com/sulkta/straw/feature/channel/ChannelViewModel.kt
View file

@ -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
}

35
strawApp/src/main/kotlin/com/sulkta/straw/feature/dataimport/SettingsImport.kt
View file

@ -110,7 +110,13 @@ object SettingsImport {
suspend fun run(context: Context, zipUri: Uri): Result<ImportResult> =
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<Pair<Long, String>>()
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,10 +492,17 @@ object SettingsImport {
for ((key, cat) in targets) {
val want = prefs.boolOrNull(key) ?: continue
val have = cat in current
if (want != have) settings.toggle(cat)
// 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++
}
}
}
// Default resolution: NewPipe values like "720p60", "1080p", "Best
// resolution". Map down to Straw's discrete ceilings.

8
strawApp/src/main/kotlin/com/sulkta/straw/feature/detail/VideoDetailViewModel.kt
View file

@ -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
}

5
strawApp/src/main/kotlin/com/sulkta/straw/feature/player/PlayerScreen.kt
View file

@ -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
}

31
strawApp/src/main/kotlin/com/sulkta/straw/feature/search/SearchViewModel.kt
View file

@ -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 {