From 7d58f451bd38b727f2cb6b02f8ee327276baa54f Mon Sep 17 00:00:00 2001 From: Tpt Date: Sat, 15 Jan 2022 12:49:02 +0100 Subject: [PATCH] Runs LLVM address sanitizer and fixes found bugs --- .github/workflows/tests.yml | 12 ++++ Cargo.lock | 24 +++---- lib/src/storage/backend/rocksdb.rs | 69 +++++++++++++------ lib/tests/store.rs | 48 ++++++++++++- rocksdb-sys/api/c.cc | 9 ++- .../sparql/nested_expression.rq | 2 +- 6 files changed, 129 insertions(+), 35 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e1ebdbb4..d3abc966 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -44,6 +44,18 @@ jobs: env: RUST_BACKTRACE: 1 + address_sanitizer: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + submodules: true + - run: rustup update && rustup toolchain install nightly + - run: cargo +nightly test --tests --target x86_64-unknown-linux-gnu --workspace --exclude pyoxigraph --exclude oxigraph_testsuite + env: + RUST_BACKTRACE: 1 + RUSTFLAGS: -Z sanitizer=address + test_windows: runs-on: windows-latest steps: diff --git a/Cargo.lock b/Cargo.lock index 3c3c45d1..43e39fef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -177,9 +177,9 @@ dependencies = [ [[package]] name = "clap" -version = "3.0.6" +version = "3.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1957aa4a5fb388f0a0a73ce7556c5b42025b874e5cdc2c670775e346e97adec0" +checksum = "12e8611f9ae4e068fa3e56931fded356ff745e70987ff76924a6e0ab1c8ef2e3" dependencies = [ "atty", "bitflags", @@ -436,9 +436,9 @@ dependencies = [ [[package]] name = "getrandom" -version = "0.2.3" +version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fcd999463524c52659517fe2cea98493cfe485d10565e7b0fb07dbba7ad2753" +checksum = "418d37c8b1d42553c93648be529cb70f920d3baf8ef469b74b9638df426e0b4c" dependencies = [ "cfg-if", "js-sys", @@ -638,9 +638,9 @@ checksum = "1b03d17f364a3a042d5e5d46b053bbbf82c92c9430c592dd4c064dc6ee997125" [[package]] name = "libloading" -version = "0.7.2" +version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "afe203d669ec979b7128619bae5a63b7b42e9203c1b29146079ee05e2f604b52" +checksum = "efbc0f03f9a775e9f6aed295c6a1ba2253c5757a9e03d55c6caa46a681abcddd" dependencies = [ "cfg-if", "winapi 0.3.9", @@ -782,9 +782,9 @@ dependencies = [ [[package]] name = "openssl-probe" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "28988d872ab76095a6e6ac88d99b54fd267702734fd7ffe610ca27f533ddb95a" +checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" [[package]] name = "openssl-sys" @@ -870,7 +870,7 @@ dependencies = [ name = "oxigraph_server" version = "0.3.0-dev" dependencies = [ - "clap 3.0.6", + "clap 3.0.7", "oxhttp", "oxigraph", "oxiri", @@ -883,7 +883,7 @@ name = "oxigraph_testsuite" version = "0.3.0-dev" dependencies = [ "anyhow", - "clap 3.0.6", + "clap 3.0.7", "criterion", "oxigraph", "text-diff", @@ -1513,9 +1513,9 @@ checksum = "533494a8f9b724d33625ab53c6c4800f7cc445895924a8ef649222dcb76e938b" [[package]] name = "smallvec" -version = "1.7.0" +version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ecab6c735a6bb4139c0caafd0cc3635748bbb3acf4550e8138122099251f309" +checksum = "f2dd574626839106c320a323308629dcb1acfc96e32a8cba364ddc61ac23ee83" [[package]] name = "sophia_api" diff --git a/lib/src/storage/backend/rocksdb.rs b/lib/src/storage/backend/rocksdb.rs index 63cffd65..b5461ff2 100644 --- a/lib/src/storage/backend/rocksdb.rs +++ b/lib/src/storage/backend/rocksdb.rs @@ -252,7 +252,16 @@ impl Db { .as_ptr(), cf_options.as_ptr() as *const *const rocksdb_options_t, cf_handles.as_mut_ptr(), - ))?; + )) + .map_err(|e| { + for cf_option in &cf_options { + rocksdb_options_destroy(*cf_option); + } + rocksdb_transactiondb_options_destroy(transactiondb_options); + rocksdb_options_destroy(options); + rocksdb_block_based_options_destroy(block_based_table_options); + e + })?; assert!(!db.is_null(), "rocksdb_create returned null"); for handle in &cf_handles { assert!( @@ -375,13 +384,11 @@ impl Db { ); transaction }; - let read_options = unsafe { + let (read_options, snapshot) = unsafe { let options = rocksdb_readoptions_create_copy(self.0.read_options); - rocksdb_readoptions_set_snapshot( - options, - rocksdb_transaction_get_snapshot(transaction), - ); - options + let snapshot = rocksdb_transaction_get_snapshot(transaction); + rocksdb_readoptions_set_snapshot(options, snapshot); + (options, snapshot) }; let result = f(Transaction { transaction: Rc::new(transaction), @@ -391,19 +398,21 @@ impl Db { match result { Ok(result) => { unsafe { - ffi_result!(rocksdb_transaction_commit_with_status(transaction)) - .map_err(StorageError::from)?; + let r = ffi_result!(rocksdb_transaction_commit_with_status(transaction)); rocksdb_transaction_destroy(transaction); rocksdb_readoptions_destroy(read_options); + free(snapshot as *mut c_void); + r.map_err(StorageError::from)?; // We make sure to also run destructors if the commit fails } return Ok(result); } Err(e) => { unsafe { - ffi_result!(rocksdb_transaction_rollback_with_status(transaction)) - .map_err(StorageError::from)?; + let r = ffi_result!(rocksdb_transaction_rollback_with_status(transaction)); rocksdb_transaction_destroy(transaction); rocksdb_readoptions_destroy(read_options); + free(snapshot as *mut c_void); + r.map_err(StorageError::from)?; // We make sure to also run destructors if the commit fails } // We look for the root error let mut error: &(dyn Error + 'static) = &e; @@ -511,7 +520,11 @@ impl Db { ffi_result!(rocksdb_sstfilewriter_open_with_status( writer, path_to_cstring(&path)?.as_ptr() - ))?; + )) + .map_err(|e| { + rocksdb_sstfilewriter_destroy(writer); + e + })?; Ok(SstFileWriter { writer, path }) } } @@ -748,11 +761,11 @@ impl Transaction<'_> { } } - pub fn contains_key_for_update( + pub fn get_for_update( &self, column_family: &ColumnFamily, key: &[u8], - ) -> Result { + ) -> Result, StorageError> { unsafe { let slice = ffi_result!(rocksdb_transaction_get_for_update_pinned_cf_with_status( *self.transaction, @@ -761,10 +774,22 @@ impl Transaction<'_> { key.as_ptr() as *const c_char, key.len() ))?; - Ok(!slice.is_null()) + Ok(if slice.is_null() { + None + } else { + Some(PinnableSlice(slice)) + }) } } + pub fn contains_key_for_update( + &self, + column_family: &ColumnFamily, + key: &[u8], + ) -> Result { + Ok(self.get_for_update(column_family, key)?.is_some()) //TODO: optimize + } + pub fn insert( &mut self, column_family: &ColumnFamily, @@ -983,19 +1008,23 @@ unsafe impl Sync for ErrorStatus {} impl Drop for ErrorStatus { fn drop(&mut self) { - unsafe { - free(self.0.string as *mut c_void); + if self.0.string.is_null() { + unsafe { + free(self.0.string as *mut c_void); + } } } } impl fmt::Display for ErrorStatus { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str( + f.write_str(if self.0.string.is_null() { + "Unknown error" + } else { unsafe { CStr::from_ptr(self.0.string) } .to_str() - .map_err(|_| fmt::Error)?, - ) + .map_err(|_| fmt::Error)? + }) } } diff --git a/lib/tests/store.rs b/lib/tests/store.rs index ed840ef1..6dd0ec3b 100644 --- a/lib/tests/store.rs +++ b/lib/tests/store.rs @@ -2,8 +2,13 @@ use oxigraph::io::{DatasetFormat, GraphFormat}; use oxigraph::model::vocab::{rdf, xsd}; use oxigraph::model::*; use oxigraph::store::Store; +use rand::random; +use std::env::temp_dir; use std::error::Error; -use std::io::Cursor; +use std::fs::{create_dir, remove_dir_all, File}; +use std::io::{Cursor, Write}; +use std::iter::once; +use std::path::PathBuf; use std::process::Command; const DATA: &str = r#" @@ -194,6 +199,33 @@ fn test_bulk_load_on_existing_delete_overrides_the_delete() -> Result<(), Box Result<(), Box> { + let dir = TempDir::default(); + create_dir(&dir.0)?; + { + File::create(dir.0.join("CURRENT"))?.write_all(b"foo")?; + } + assert!(Store::open(&dir.0).is_err()); + Ok(()) +} + +#[test] +fn test_bad_stt_open() -> Result<(), Box> { + let dir = TempDir::default(); + let store = Store::open(&dir.0)?; + remove_dir_all(&dir.0)?; + assert!(store + .bulk_extend(once(Quad { + subject: NamedNode::new_unchecked("http://example.com/s").into(), + predicate: NamedNode::new_unchecked("http://example.com/p"), + object: NamedNode::new_unchecked("http://example.com/o").into(), + graph_name: GraphName::DefaultGraph + })) + .is_err()); + Ok(()) +} + #[test] #[cfg(target_os = "linux")] fn test_backward_compatibility() -> Result<(), Box> { @@ -229,3 +261,17 @@ fn reset_dir(dir: &str) -> Result<(), Box> { .success()); Ok(()) } + +struct TempDir(PathBuf); + +impl Default for TempDir { + fn default() -> Self { + Self(temp_dir().join(format!("oxigraph-test-{}", random::()))) + } +} + +impl Drop for TempDir { + fn drop(&mut self) { + let _ = remove_dir_all(&self.0); + } +} diff --git a/rocksdb-sys/api/c.cc b/rocksdb-sys/api/c.cc index e5e3ec43..e4b61220 100644 --- a/rocksdb-sys/api/c.cc +++ b/rocksdb-sys/api/c.cc @@ -5,7 +5,14 @@ static bool SaveStatus(rocksdb_status_t* target, const Status source) { target->code = static_cast(source.code()); target->subcode = static_cast(source.subcode()); target->severity = static_cast(source.severity()); - target->string = source.ok() ? nullptr : source.ToString().c_str(); + if(source.ok()) { + target->string = nullptr; + } else { + std::string msg = source.ToString(); + char *string = new char[msg.size() + 1]; //we need extra char for NUL + memcpy(string, msg.c_str(), msg.size() + 1); + target->string = string; + } return !source.ok(); } diff --git a/testsuite/oxigraph-tests/sparql/nested_expression.rq b/testsuite/oxigraph-tests/sparql/nested_expression.rq index 374e3967..889b4c33 100644 --- a/testsuite/oxigraph-tests/sparql/nested_expression.rq +++ b/testsuite/oxigraph-tests/sparql/nested_expression.rq @@ -1,4 +1,4 @@ SELECT * WHERE { ?s ?p ?o . - FILTER((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((true)))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))) + FILTER((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((true)))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))) }