Skip to main content

max / audiofiles

storage: enforce root-name uniqueness, harden sample-info reads (ultra-fuzz run 3) Root VFS-node name uniqueness was guarded only by a non-atomic COUNT-then-INSERT check; SQLite's UNIQUE treats each NULL parent_id as distinct, and the DB is shared across the CLAP host and GUI threads, so two concurrent same-name root creates could both pass and both insert. Add a partial unique index (M029) on (vfs_id, name) WHERE parent_id IS NULL so the race is unrepresentable; keep the COUNT check as the friendly-error fast path and map any UNIQUE violation (the race loser) to NameConflict via a shared insert helper. build_sample_info read sample_rate/channels as u32/u16, so a valid-but-wide stored value surfaced as a query error and was lumped into the "no analysis row" (0,0) fallback. Read as i64 and range-check explicitly, logging genuine corruption so a device profile can't silently drop a sample on mystery zeros; only QueryReturnedNoRows stays a silent fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-19 21:35 UTC
Commit: 3057e117cd64d3bd4e991fe30c1f4b767273993a
Parent: 035db7b
3 files changed, +94 insertions, -15 deletions
M crates/audiofiles-browser/src/backend/direct.rs +20 -3
@@ -289,7 +289,13 @@ fn build_sample_info(
289 289 store: &audiofiles_core::store::SampleStore,
290 290 item: &ExportItem,
291 291 ) -> audiofiles_rhai::types::RhaiSampleInfo {
292 - // Query audio_analysis for sample_rate and channels
292 + // Query audio_analysis for sample_rate and channels. Read the integers as
293 + // i64 so a valid-but-out-of-range stored value isn't reported as a query
294 + // error and lumped in with the legitimate "no analysis row yet" case; range
295 + // problems are surfaced explicitly below instead. Only QueryReturnedNoRows
296 + // is a silent (0,0) fallback — a genuine read error or a corrupt value
297 + // (negative / oversized rate or channels) is logged so a device profile
298 + // can't silently drop the sample on mystery zeros.
293 299 let (sample_rate, channels, duration) = db
294 300 .conn()
295 301 .query_row(
@@ -297,12 +303,23 @@ fn build_sample_info(
297 303 [&item.hash],
298 304 |row| {
299 305 Ok((
300 - row.get::<_, u32>(0)?,
301 - row.get::<_, u16>(1)?,
306 + row.get::<_, i64>(0)?,
307 + row.get::<_, i64>(1)?,
302 308 row.get::<_, f64>(2)?,
303 309 ))
304 310 },
305 311 )
312 + .map(|(sr, ch, dur)| {
313 + let sample_rate = u32::try_from(sr).unwrap_or_else(|_| {
314 + tracing::warn!("Corrupt sample_rate {sr} for {}", &item.hash[..8]);
315 + 0
316 + });
317 + let channels = u16::try_from(ch).unwrap_or_else(|_| {
318 + tracing::warn!("Corrupt channels {ch} for {}", &item.hash[..8]);
319 + 0
320 + });
321 + (sample_rate, channels, dur)
322 + })
306 323 .unwrap_or_else(|e| {
307 324 if !matches!(e, rusqlite::Error::QueryReturnedNoRows) {
308 325 tracing::warn!("Failed to query audio_analysis for {}: {e}", &item.hash[..8]);
M crates/audiofiles-core/src/db.rs +20 -6
@@ -1462,6 +1462,19 @@ CREATE INDEX IF NOT EXISTS idx_analysis_peak_db ON audio_analysis(peak_db);
1462 1462 CREATE INDEX IF NOT EXISTS idx_analysis_duration ON audio_analysis(duration);
1463 1463 "#;
1464 1464
1465 + const MIGRATION_029: &str = r#"
1466 + -- Enforce root-node name uniqueness at the engine level. SQLite's UNIQUE
1467 + -- treats every NULL as distinct, so the table-level UNIQUE(vfs_id, parent_id,
1468 + -- name) does NOT cover root nodes (parent_id IS NULL); uniqueness there was
1469 + -- guarded only by a COUNT-then-INSERT check, which is not atomic. Because the
1470 + -- DB is shared across the CLAP host thread and the GUI thread, two concurrent
1471 + -- same-name root creates could both pass the COUNT and both insert. A partial
1472 + -- unique index makes that race unrepresentable; the COUNT check stays as the
1473 + -- friendly-error fast path. Additive and idempotent.
1474 + CREATE UNIQUE INDEX IF NOT EXISTS idx_vfs_root_name
1475 + ON vfs_nodes(vfs_id, name) WHERE parent_id IS NULL;
1476 + "#;
1477 +
1465 1478 /// Register `hash_row_id(salt, key) -> TEXT` as a deterministic SQLite
1466 1479 /// function on the given connection. Used by the M018 sync triggers so the
1467 1480 /// `sync_changelog.row_id` field never carries cleartext content (tag strings,
@@ -1581,6 +1594,7 @@ impl Database {
1581 1594 MIGRATION_026,
1582 1595 MIGRATION_027,
1583 1596 MIGRATION_028,
1597 + MIGRATION_029,
1584 1598 ];
1585 1599
1586 1600 for (i, sql) in MIGRATIONS.iter().enumerate() {
@@ -1792,7 +1806,7 @@ mod tests {
1792 1806 .conn()
1793 1807 .query_row("PRAGMA user_version", [], |row| row.get(0))
1794 1808 .unwrap();
1795 - assert_eq!(version, 28);
1809 + assert_eq!(version, 29);
1796 1810 }
1797 1811
1798 1812 #[test]
@@ -1803,7 +1817,7 @@ mod tests {
1803 1817 .conn()
1804 1818 .query_row("PRAGMA user_version", [], |row| row.get(0))
1805 1819 .unwrap();
1806 - assert_eq!(version, 28);
1820 + assert_eq!(version, 29);
1807 1821 }
1808 1822
1809 1823 /// Open a fresh file-backed DB, close, reopen. The second open re-enters
@@ -1822,7 +1836,7 @@ mod tests {
1822 1836 .conn()
1823 1837 .query_row("PRAGMA user_version", [], |row| row.get(0))
1824 1838 .unwrap();
1825 - assert_eq!(version, 28);
1839 + assert_eq!(version, 29);
1826 1840 }
1827 1841
1828 1842 /// Simulates the worst-case recovery path: a prior partial migration left
@@ -1866,7 +1880,7 @@ mod tests {
1866 1880 .conn()
1867 1881 .query_row("PRAGMA user_version", [], |row| row.get(0))
1868 1882 .unwrap();
1869 - assert_eq!(version, 28);
1883 + assert_eq!(version, 29);
1870 1884 }
1871 1885
1872 1886 /// M018 contract: the `sync_changelog.row_id` for sensitive tables must
@@ -2088,7 +2102,7 @@ mod tests {
2088 2102 let initial_version: i32 = conn
2089 2103 .query_row("PRAGMA user_version", [], |row| row.get(0))
2090 2104 .unwrap();
2091 - assert_eq!(initial_version, 28);
2105 + assert_eq!(initial_version, 29);
2092 2106
2093 2107 let batch = format!(
2094 2108 "BEGIN;\n{}\nPRAGMA user_version = 999;\nCOMMIT;",
@@ -2151,7 +2165,7 @@ mod tests {
2151 2165 .conn()
2152 2166 .query_row("PRAGMA user_version", [], |row| row.get(0))
2153 2167 .unwrap();
2154 - assert_eq!(version, 28);
2168 + assert_eq!(version, 29);
2155 2169 }
2156 2170
2157 2171 #[test]
M crates/audiofiles-core/src/vfs.rs +54 -6
@@ -231,12 +231,13 @@ pub fn create_directory(
231 231 validate_node_name(name)?;
232 232 check_root_name_conflict(db, vfs_id, parent_id, name)?;
233 233 let now = unix_now();
234 - db.conn().execute(
234 + insert_node_or_conflict(
235 + db,
235 236 "INSERT INTO vfs_nodes (vfs_id, parent_id, name, node_type, created_at)
236 237 VALUES (?1, ?2, ?3, 'directory', ?4)",
237 238 rusqlite::params![vfs_id, parent_id, name, now],
238 - )?;
239 - Ok(NodeId::from(db.conn().last_insert_rowid()))
239 + name,
240 + )
240 241 }
241 242
242 243 /// Create a sample link node pointing to a content-addressed hash. Returns the new node ID.
@@ -251,12 +252,32 @@ pub fn create_sample_link(
251 252 validate_node_name(name)?;
252 253 check_root_name_conflict(db, vfs_id, parent_id, name)?;
253 254 let now = unix_now();
254 - db.conn().execute(
255 + insert_node_or_conflict(
256 + db,
255 257 "INSERT INTO vfs_nodes (vfs_id, parent_id, name, node_type, sample_hash, created_at)
256 258 VALUES (?1, ?2, ?3, 'sample', ?4, ?5)",
257 259 rusqlite::params![vfs_id, parent_id, name, sample_hash, now],
258 - )?;
259 - Ok(NodeId::from(db.conn().last_insert_rowid()))
260 + name,
261 + )
262 + }
263 +
264 + /// Insert a vfs_nodes row, mapping a UNIQUE-constraint violation (a sibling or
265 + /// root name collision — including one lost to a concurrent insert past the
266 + /// non-atomic `check_root_name_conflict` fast path) to a clean
267 + /// [`CoreError::NameConflict`] rather than a raw SQLite error.
268 + fn insert_node_or_conflict(
269 + db: &Database,
270 + sql: &str,
271 + params: impl rusqlite::Params,
272 + name: &str,
273 + ) -> Result<NodeId> {
274 + match db.conn().execute(sql, params) {
275 + Ok(_) => Ok(NodeId::from(db.conn().last_insert_rowid())),
276 + Err(e) if e.to_string().contains("UNIQUE") => {
277 + Err(CoreError::NameConflict(name.to_string()))
278 + }
279 + Err(e) => Err(e.into()),
280 + }
260 281 }
261 282
262 283 /// List direct children of a directory (or root if `parent_id` is `None`), sorted directories-first then by name.
@@ -755,6 +776,33 @@ mod tests {
755 776 }
756 777
757 778 #[test]
779 + fn root_name_uniqueness_enforced_by_index() {
780 + // The partial unique index (M029) is the backstop for the non-atomic
781 + // COUNT-then-INSERT check: a second same-name root insert must be
782 + // rejected by the DB even when it bypasses the Rust-side check — the
783 + // concurrent-insert race past check_root_name_conflict.
784 + let db = setup();
785 + let vfs_id = create_vfs(&db, "Lib").unwrap();
786 + db.conn()
787 + .execute(
788 + "INSERT INTO vfs_nodes (vfs_id, parent_id, name, node_type, created_at)
789 + VALUES (?1, NULL, 'Drums', 'directory', 0)",
790 + rusqlite::params![vfs_id],
791 + )
792 + .unwrap();
793 + let dup = db.conn().execute(
794 + "INSERT INTO vfs_nodes (vfs_id, parent_id, name, node_type, created_at)
795 + VALUES (?1, NULL, 'Drums', 'directory', 0)",
796 + rusqlite::params![vfs_id],
797 + );
798 + assert!(
799 + dup.is_err(),
800 + "partial unique index must reject a duplicate root name"
801 + );
802 + assert!(dup.unwrap_err().to_string().contains("UNIQUE"));
803 + }
804 +
805 + #[test]
758 806 fn breadcrumb_trail() {
759 807 let db = setup();
760 808 let vfs_id = create_vfs(&db, "Lib").unwrap();