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)
96 lines
3.9 KiB
Rust
96 lines
3.9 KiB
Rust
// Runtime bootstrap. Called once from Kotlin's StrawApp.onCreate via
|
|
// init_logging(), and again before every strawcore call. Wires the
|
|
// strawcore-core Downloader + Localization singleton so the extractor
|
|
// has an HTTP client to use.
|
|
//
|
|
// Round-4 audit HIGH-1: the prior shape used `Once::call_once` and
|
|
// silently swallowed errors. If the FIRST call ran while the network
|
|
// stack wasn't ready (cold boot in airplane mode, SELinux denial on
|
|
// first TLS init, transient resolver failure), the Once slot was
|
|
// consumed, NewPipe::init_full never ran, and every subsequent
|
|
// search/streamInfo/channelInfo returned DownloaderMissing for the
|
|
// rest of the process lifetime.
|
|
//
|
|
// New shape: use an AtomicBool to track success. Only "success" closes
|
|
// the door. On failure we retry — rate-limited so a persistently-broken
|
|
// network doesn't hammer reqwest::Client::new() on every call.
|
|
|
|
use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
|
|
use std::sync::{Arc, Mutex};
|
|
use std::time::{SystemTime, UNIX_EPOCH};
|
|
|
|
use strawcore_core::downloader::ReqwestDownloader;
|
|
use strawcore_core::localization::{ContentCountry, Localization};
|
|
use strawcore_core::newpipe::NewPipe;
|
|
|
|
static INITIALIZED: AtomicBool = AtomicBool::new(false);
|
|
static LAST_ATTEMPT_MS: AtomicU64 = AtomicU64::new(0);
|
|
// Guards the actual init attempt so concurrent calls don't all try
|
|
// to build the downloader in parallel; serial retry is the goal.
|
|
static INIT_LOCK: Mutex<()> = Mutex::new(());
|
|
|
|
/// Min ms between retries when init has failed. 5s — enough that a
|
|
/// hot loop of failed searches doesn't pin a CPU on reqwest setup,
|
|
/// short enough that a user who toggled airplane mode off recovers
|
|
/// within one tap.
|
|
const RETRY_BACKOFF_MS: u64 = 5_000;
|
|
|
|
fn now_ms() -> u64 {
|
|
SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.map(|d| d.as_millis() as u64)
|
|
.unwrap_or(0)
|
|
}
|
|
|
|
pub fn ensure_initialized() {
|
|
// Fast path: already initialized. Single Acquire load.
|
|
if INITIALIZED.load(Ordering::Acquire) {
|
|
return;
|
|
}
|
|
// 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(
|
|
Arc::new(dl),
|
|
Localization::default(),
|
|
ContentCountry::default(),
|
|
);
|
|
INITIALIZED.store(true, Ordering::Release);
|
|
// Clear LAST_ATTEMPT_MS so a future hypothetical
|
|
// re-init path (none today) wouldn't see cooldown
|
|
// bleed from this success.
|
|
LAST_ATTEMPT_MS.store(0, Ordering::Release);
|
|
log::info!("strawcore-core: downloader + localization initialized");
|
|
}
|
|
Err(e) => {
|
|
// Stamp the timestamp on FAILURE only, so the next
|
|
// caller within RETRY_BACKOFF_MS skips, but a successful
|
|
// attempt isn't reflected in the backoff state.
|
|
LAST_ATTEMPT_MS.store(now, Ordering::Release);
|
|
log::error!("strawcore-core: downloader init failed (will retry on next call)");
|
|
let _ = e;
|
|
}
|
|
}
|
|
}
|