From a46c3d2502c2528d0db1a8caef01e35866069953 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 8 Mar 2024 19:20:14 +0100 Subject: [PATCH] [rm-sled] Replace unsafe code with ouroboros in sqlite adapter --- Cargo.lock | 57 +++++++++++++++ Cargo.nix | 90 ++++++++++++++++++++++- Cargo.toml | 1 + src/db/Cargo.toml | 1 + src/db/sqlite_adapter.rs | 154 ++++++++++++--------------------------- 5 files changed, 194 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 284e2276..59b1fda4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -74,6 +74,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "aliasable" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "250f629c0161ad8107cf89319e990051fae62832fd343083bea452d93e2205fd" + [[package]] name = "allocator-api2" version = "0.2.16" @@ -1443,6 +1449,7 @@ dependencies = [ "heed", "hexdump", "mktemp", + "ouroboros", "rusqlite", "tracing", ] @@ -2775,6 +2782,31 @@ dependencies = [ "num-traits", ] +[[package]] +name = "ouroboros" +version = "0.18.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "97b7be5a8a3462b752f4be3ff2b2bf2f7f1d00834902e46be2a4d68b87b0573c" +dependencies = [ + "aliasable", + "ouroboros_macro", + "static_assertions", +] + +[[package]] +name = "ouroboros_macro" +version = "0.18.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b645dcde5f119c2c454a92d0dfa271a2a3b205da92e4292a68ead4bdbfde1f33" +dependencies = [ + "heck 0.4.1", + "itertools 0.12.1", + "proc-macro2", + "proc-macro2-diagnostics", + "quote", + "syn 2.0.48", +] + [[package]] name = "outref" version = "0.5.1" @@ -3102,6 +3134,19 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proc-macro2-diagnostics" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af066a9c399a26e020ada66a034357a868728e72cd426f3adcd35f80d88d88c8" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.48", + "version_check", + "yansi", +] + [[package]] name = "prometheus" version = "0.13.3" @@ -3834,6 +3879,12 @@ dependencies = [ "der", ] +[[package]] +name = "static_assertions" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" + [[package]] name = "static_init" version = "1.0.3" @@ -4844,6 +4895,12 @@ version = "0.8.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "53be06678ed9e83edb1745eb72efc0bbcd7b5c3c35711a860906aed827a13d61" +[[package]] +name = "yansi" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c2861d76f58ec8fc95708b9b1e417f7b12fd72ad33c01fa6886707092dea0d3" + [[package]] name = "zerocopy" version = "0.7.32" diff --git a/Cargo.nix b/Cargo.nix index 0bc28ecd..a88f029b 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -34,7 +34,7 @@ args@{ ignoreLockHash, }: let - nixifiedLockHash = "23e1504df44ec18cfc5c872c858154304c16da2a6c1f7c9f06608ef833815f30"; + nixifiedLockHash = "17e620fad07e725e301488a3746ca3512d8785ce3077f634adde82e9e4eab2bb"; workspaceSrc = if args.workspaceSrc == null then ./. else args.workspaceSrc; currentLockHash = builtins.hashFile "sha256" (workspaceSrc + /Cargo.lock); lockHashIgnored = if ignoreLockHash @@ -176,6 +176,17 @@ in }; }); + "registry+https://github.com/rust-lang/crates.io-index".aliasable."0.1.3" = overridableMkRustCrate (profileName: rec { + name = "aliasable"; + version = "0.1.3"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "250f629c0161ad8107cf89319e990051fae62832fd343083bea452d93e2205fd"; }; + features = builtins.concatLists [ + [ "alloc" ] + [ "default" ] + ]; + }); + "registry+https://github.com/rust-lang/crates.io-index".allocator-api2."0.2.16" = overridableMkRustCrate (profileName: rec { name = "allocator-api2"; version = "0.2.16"; @@ -2095,6 +2106,7 @@ in err_derive = (buildRustPackages."registry+https://github.com/rust-lang/crates.io-index".err-derive."0.3.1" { profileName = "__noProfile"; }).out; ${ if rootFeatures' ? "garage/default" || rootFeatures' ? "garage/lmdb" || rootFeatures' ? "garage_db/default" || rootFeatures' ? "garage_db/heed" || rootFeatures' ? "garage_db/lmdb" || rootFeatures' ? "garage_model/default" || rootFeatures' ? "garage_model/lmdb" then "heed" else null } = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".heed."0.11.0" { inherit profileName; }).out; hexdump = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".hexdump."0.1.1" { inherit profileName; }).out; + ouroboros = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".ouroboros."0.18.3" { inherit profileName; }).out; ${ if rootFeatures' ? "garage/bundled-libs" || rootFeatures' ? "garage/default" || rootFeatures' ? "garage/sqlite" || rootFeatures' ? "garage_db/bundled-libs" || rootFeatures' ? "garage_db/default" || rootFeatures' ? "garage_db/rusqlite" || rootFeatures' ? "garage_db/sqlite" || rootFeatures' ? "garage_model/default" || rootFeatures' ? "garage_model/sqlite" then "rusqlite" else null } = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".rusqlite."0.30.0" { inherit profileName; }).out; tracing = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".tracing."0.1.40" { inherit profileName; }).out; }; @@ -2510,7 +2522,7 @@ in registry = "registry+https://github.com/rust-lang/crates.io-index"; src = fetchCratesIo { inherit name version; sha256 = "95505c38b4572b2d910cecb0281560f54b440a19336cbbcb27bf6ce6adc6f5a8"; }; features = builtins.concatLists [ - (lib.optional (rootFeatures' ? "k2v-client/clap" || rootFeatures' ? "k2v-client/cli") "default") + [ "default" ] ]; }); @@ -3958,6 +3970,40 @@ in }; }); + "registry+https://github.com/rust-lang/crates.io-index".ouroboros."0.18.3" = overridableMkRustCrate (profileName: rec { + name = "ouroboros"; + version = "0.18.3"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "97b7be5a8a3462b752f4be3ff2b2bf2f7f1d00834902e46be2a4d68b87b0573c"; }; + features = builtins.concatLists [ + [ "default" ] + [ "std" ] + ]; + dependencies = { + aliasable = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".aliasable."0.1.3" { inherit profileName; }).out; + ouroboros_macro = (buildRustPackages."registry+https://github.com/rust-lang/crates.io-index".ouroboros_macro."0.18.3" { profileName = "__noProfile"; }).out; + static_assertions = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".static_assertions."1.1.0" { inherit profileName; }).out; + }; + }); + + "registry+https://github.com/rust-lang/crates.io-index".ouroboros_macro."0.18.3" = overridableMkRustCrate (profileName: rec { + name = "ouroboros_macro"; + version = "0.18.3"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "b645dcde5f119c2c454a92d0dfa271a2a3b205da92e4292a68ead4bdbfde1f33"; }; + features = builtins.concatLists [ + [ "std" ] + ]; + dependencies = { + heck = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".heck."0.4.1" { inherit profileName; }).out; + itertools = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".itertools."0.12.1" { inherit profileName; }).out; + proc_macro2 = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".proc-macro2."1.0.78" { inherit profileName; }).out; + proc_macro2_diagnostics = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".proc-macro2-diagnostics."0.10.1" { inherit profileName; }).out; + quote = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".quote."1.0.35" { inherit profileName; }).out; + syn = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".syn."2.0.48" { inherit profileName; }).out; + }; + }); + "registry+https://github.com/rust-lang/crates.io-index".outref."0.5.1" = overridableMkRustCrate (profileName: rec { name = "outref"; version = "0.5.1"; @@ -4394,6 +4440,27 @@ in }; }); + "registry+https://github.com/rust-lang/crates.io-index".proc-macro2-diagnostics."0.10.1" = overridableMkRustCrate (profileName: rec { + name = "proc-macro2-diagnostics"; + version = "0.10.1"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "af066a9c399a26e020ada66a034357a868728e72cd426f3adcd35f80d88d88c8"; }; + features = builtins.concatLists [ + [ "colors" ] + [ "default" ] + [ "yansi" ] + ]; + dependencies = { + proc_macro2 = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".proc-macro2."1.0.78" { inherit profileName; }).out; + quote = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".quote."1.0.35" { inherit profileName; }).out; + syn = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".syn."2.0.48" { inherit profileName; }).out; + yansi = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".yansi."1.0.0" { inherit profileName; }).out; + }; + buildDependencies = { + version_check = (buildRustPackages."registry+https://github.com/rust-lang/crates.io-index".version_check."0.9.4" { profileName = "__noProfile"; }).out; + }; + }); + "registry+https://github.com/rust-lang/crates.io-index".prometheus."0.13.3" = overridableMkRustCrate (profileName: rec { name = "prometheus"; version = "0.13.3"; @@ -5466,6 +5533,13 @@ in }; }); + "registry+https://github.com/rust-lang/crates.io-index".static_assertions."1.1.0" = overridableMkRustCrate (profileName: rec { + name = "static_assertions"; + version = "1.1.0"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f"; }; + }); + "registry+https://github.com/rust-lang/crates.io-index".static_init."1.0.3" = overridableMkRustCrate (profileName: rec { name = "static_init"; version = "1.0.3"; @@ -6957,6 +7031,18 @@ in ]; }); + "registry+https://github.com/rust-lang/crates.io-index".yansi."1.0.0" = overridableMkRustCrate (profileName: rec { + name = "yansi"; + version = "1.0.0"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "6c2861d76f58ec8fc95708b9b1e417f7b12fd72ad33c01fa6886707092dea0d3"; }; + features = builtins.concatLists [ + [ "alloc" ] + [ "default" ] + [ "std" ] + ]; + }); + "registry+https://github.com/rust-lang/crates.io-index".zerocopy."0.7.32" = overridableMkRustCrate (profileName: rec { name = "zerocopy"; version = "0.7.32"; diff --git a/Cargo.toml b/Cargo.toml index f40e8738..88df70e6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ md-5 = "0.10" mktemp = "0.5" nix = { version = "0.27", default-features = false, features = ["fs"] } nom = "7.1" +ouroboros = "0.18" parse_duration = "2.1" pin-project = "1.0.12" pnet_datalink = "0.34" diff --git a/src/db/Cargo.toml b/src/db/Cargo.toml index a8f6d586..dae4e7a5 100644 --- a/src/db/Cargo.toml +++ b/src/db/Cargo.toml @@ -14,6 +14,7 @@ path = "lib.rs" [dependencies] err-derive.workspace = true hexdump.workspace = true +ouroboros.workspace = true tracing.workspace = true heed = { workspace = true, optional = true } diff --git a/src/db/sqlite_adapter.rs b/src/db/sqlite_adapter.rs index 6c556c97..8394b6a6 100644 --- a/src/db/sqlite_adapter.rs +++ b/src/db/sqlite_adapter.rs @@ -1,11 +1,9 @@ use core::ops::Bound; use std::borrow::BorrowMut; -use std::marker::PhantomPinned; -use std::pin::Pin; -use std::ptr::NonNull; use std::sync::{Arc, Mutex, MutexGuard}; +use ouroboros::self_referencing; use rusqlite::{params, Connection, Rows, Statement, Transaction}; use crate::{ @@ -197,7 +195,7 @@ impl IDb for SqliteDb { let tree = this.get_tree(tree)?; let sql = format!("SELECT k, v FROM {} ORDER BY k ASC", tree); - DbValueIterator::make(this, &sql, []) + make_iterator(this, &sql, []) } fn iter_rev(&self, tree: usize) -> Result> { @@ -207,7 +205,7 @@ impl IDb for SqliteDb { let tree = this.get_tree(tree)?; let sql = format!("SELECT k, v FROM {} ORDER BY k DESC", tree); - DbValueIterator::make(this, &sql, []) + make_iterator(this, &sql, []) } fn range<'r>( @@ -230,7 +228,7 @@ impl IDb for SqliteDb { .map(|x| x as &dyn rusqlite::ToSql) .collect::>(); - DbValueIterator::make::<&[&dyn rusqlite::ToSql]>(this, &sql, params.as_ref()) + make_iterator::<&[&dyn rusqlite::ToSql]>(this, &sql, params.as_ref()) } fn range_rev<'r>( &self, @@ -252,7 +250,7 @@ impl IDb for SqliteDb { .map(|x| x as &dyn rusqlite::ToSql) .collect::>(); - DbValueIterator::make::<&[&dyn rusqlite::ToSql]>(this, &sql, params.as_ref()) + make_iterator::<&[&dyn rusqlite::ToSql]>(this, &sql, params.as_ref()) } // ---- @@ -372,12 +370,12 @@ impl<'a> ITx for SqliteTx<'a> { fn iter(&self, tree: usize) -> TxOpResult> { let tree = self.get_tree(tree)?; let sql = format!("SELECT k, v FROM {} ORDER BY k ASC", tree); - TxValueIterator::make(self, &sql, []) + make_tx_iterator(self, &sql, []) } fn iter_rev(&self, tree: usize) -> TxOpResult> { let tree = self.get_tree(tree)?; let sql = format!("SELECT k, v FROM {} ORDER BY k DESC", tree); - TxValueIterator::make(self, &sql, []) + make_tx_iterator(self, &sql, []) } fn range<'r>( @@ -396,7 +394,7 @@ impl<'a> ITx for SqliteTx<'a> { .map(|x| x as &dyn rusqlite::ToSql) .collect::>(); - TxValueIterator::make::<&[&dyn rusqlite::ToSql]>(self, &sql, params.as_ref()) + make_tx_iterator::<&[&dyn rusqlite::ToSql]>(self, &sql, params.as_ref()) } fn range_rev<'r>( &self, @@ -414,77 +412,47 @@ impl<'a> ITx for SqliteTx<'a> { .map(|x| x as &dyn rusqlite::ToSql) .collect::>(); - TxValueIterator::make::<&[&dyn rusqlite::ToSql]>(self, &sql, params.as_ref()) + make_tx_iterator::<&[&dyn rusqlite::ToSql]>(self, &sql, params.as_ref()) } } // ---- iterators outside transactions ---- // complicated, they must hold the Statement and Row objects -// therefore quite some unsafe code (it is a self-referential struct) +// so we need a self_referencing struct -struct DbValueIterator<'a> { +// need to split in two because sequential mutable borrows are broken, +// see https://github.com/someguynamedjosh/ouroboros/issues/100 +#[self_referencing] +struct DbValueIterator1<'a> { db: MutexGuard<'a, SqliteDbInner>, - stmt: Option>, - iter: Option>, - _pin: PhantomPinned, + #[borrows(mut db)] + #[covariant] + stmt: Statement<'this>, } -impl<'a> DbValueIterator<'a> { - fn make( - db: MutexGuard<'a, SqliteDbInner>, - sql: &str, - args: P, - ) -> Result> { - let res = DbValueIterator { - db, - stmt: None, - iter: None, - _pin: PhantomPinned, - }; - let mut boxed = Box::pin(res); - trace!("make iterator with sql: {}", sql); - - // This unsafe allows us to bypass lifetime checks - let db = unsafe { NonNull::from(&boxed.db).as_ref() }; - let stmt = db.db.prepare(sql)?; - - let mut_ref = Pin::as_mut(&mut boxed); - // This unsafe allows us to write in a field of the pinned struct - unsafe { - Pin::get_unchecked_mut(mut_ref).stmt = Some(stmt); - } - - // This unsafe allows us to bypass lifetime checks - let stmt = unsafe { NonNull::from(&boxed.stmt).as_mut() }; - let iter = stmt.as_mut().unwrap().query(args)?; - - let mut_ref = Pin::as_mut(&mut boxed); - // This unsafe allows us to write in a field of the pinned struct - unsafe { - Pin::get_unchecked_mut(mut_ref).iter = Some(iter); - } - - Ok(Box::new(DbValueIteratorPin(boxed))) - } +#[self_referencing] +struct DbValueIterator<'a> { + aux: DbValueIterator1<'a>, + #[borrows(mut aux)] + #[covariant] + iter: Rows<'this>, } -impl<'a> Drop for DbValueIterator<'a> { - fn drop(&mut self) { - trace!("drop iter"); - drop(self.iter.take()); - drop(self.stmt.take()); - } +fn make_iterator<'a, P: rusqlite::Params>( + db: MutexGuard<'a, SqliteDbInner>, + sql: &str, + args: P, +) -> Result> { + let aux = DbValueIterator1::try_new(db, |db| db.db.prepare(sql))?; + let res = DbValueIterator::try_new(aux, |aux| aux.with_stmt_mut(|stmt| stmt.query(args)))?; + Ok(Box::new(res)) } -struct DbValueIteratorPin<'a>(Pin>>); - -impl<'a> Iterator for DbValueIteratorPin<'a> { +impl<'a> Iterator for DbValueIterator<'a> { type Item = Result<(Value, Value)>; fn next(&mut self) -> Option { - let mut_ref = Pin::as_mut(&mut self.0); - // This unsafe allows us to mutably access the iterator field - let next = unsafe { Pin::get_unchecked_mut(mut_ref).iter.as_mut()?.next() }; + let next = self.with_iter_mut(|iter| iter.next()); iter_next_row(next) } } @@ -493,57 +461,29 @@ impl<'a> Iterator for DbValueIteratorPin<'a> { // it's the same except we don't hold a mutex guard, // only a Statement and a Rows object +#[self_referencing] struct TxValueIterator<'a> { stmt: Statement<'a>, - iter: Option>, - _pin: PhantomPinned, + #[borrows(mut stmt)] + #[covariant] + iter: Rows<'this>, } -impl<'a> TxValueIterator<'a> { - fn make( - tx: &'a SqliteTx<'a>, - sql: &str, - args: P, - ) -> TxOpResult> { - let stmt = tx.tx.prepare(sql)?; - let res = TxValueIterator { - stmt, - iter: None, - _pin: PhantomPinned, - }; - let mut boxed = Box::pin(res); - trace!("make iterator with sql: {}", sql); - - // This unsafe allows us to bypass lifetime checks - let stmt = unsafe { NonNull::from(&boxed.stmt).as_mut() }; - let iter = stmt.query(args)?; - - let mut_ref = Pin::as_mut(&mut boxed); - // This unsafe allows us to write in a field of the pinned struct - unsafe { - Pin::get_unchecked_mut(mut_ref).iter = Some(iter); - } - - Ok(Box::new(TxValueIteratorPin(boxed))) - } +fn make_tx_iterator<'a, P: rusqlite::Params>( + tx: &'a SqliteTx<'a>, + sql: &str, + args: P, +) -> TxOpResult> { + let stmt = tx.tx.prepare(sql)?; + let res = TxValueIterator::try_new(stmt, |stmt| stmt.query(args))?; + Ok(Box::new(res)) } -impl<'a> Drop for TxValueIterator<'a> { - fn drop(&mut self) { - trace!("drop iter"); - drop(self.iter.take()); - } -} - -struct TxValueIteratorPin<'a>(Pin>>); - -impl<'a> Iterator for TxValueIteratorPin<'a> { +impl<'a> Iterator for TxValueIterator<'a> { type Item = TxOpResult<(Value, Value)>; fn next(&mut self) -> Option { - let mut_ref = Pin::as_mut(&mut self.0); - // This unsafe allows us to mutably access the iterator field - let next = unsafe { Pin::get_unchecked_mut(mut_ref).iter.as_mut()?.next() }; + let next = self.with_iter_mut(|iter| iter.next()); iter_next_row(next) } }