Runs LLVM address sanitizer and fixes found bugs

3 years ago · 7d58f451bd
parent 12c297425a
commit 7d58f451bd
6 changed files with 129 additions and 35 deletions
--- 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:
--- 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"
--- 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<bool, StorageError> {
+    ) -> Result<Option<PinnableSlice>, 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<bool, StorageError> {
+        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) {
+        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)?
+        })
    }
 }

--- 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<dy
    Ok(())
 }

+#[test]
+fn test_open_bad_dir() -> Result<(), Box<dyn Error>> {
+    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<dyn Error>> {
+    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<dyn Error>> {
@ -229,3 +261,17 @@ fn reset_dir(dir: &str) -> Result<(), Box<dyn Error>> {
        .success());
    Ok(())
 }
+
+struct TempDir(PathBuf);
+
+impl Default for TempDir {
+    fn default() -> Self {
+        Self(temp_dir().join(format!("oxigraph-test-{}", random::<u128>())))
+    }
+}
+
+impl Drop for TempDir {
+    fn drop(&mut self) {
+        let _ = remove_dir_all(&self.0);
+    }
+}
--- 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<rocksdb_status_code_t>(source.code());
    target->subcode = static_cast<rocksdb_status_subcode_t>(source.subcode());
    target->severity = static_cast<rocksdb_status_severity_t>(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();
 }

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