Merge pull request #1929 from Skgland/master

detect and prevent concurrent AtomTable use

Cargo.lock

@@ -349,6 +349,19 @@ dependencies = [
  "windows-sys 0.48.0",
 ]
 
+[[package]]
+name = "dashmap"
+version = "5.5.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6943ae99c34386c84a470c499d3414f66502a41340aa895406e0d2e4a207b91d"
+dependencies = [
+ "cfg-if",
+ "hashbrown 0.14.0",
+ "lock_api",
+ "once_cell",
+ "parking_lot_core 0.9.8",
+]
+
 [[package]]
 name = "dashu"
 version = "0.3.1"
@@ -794,6 +807,12 @@ version = "0.12.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888"
 
+[[package]]
+name = "hashbrown"
+version = "0.14.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2c6201b9ff9fd90a5a3bac2e56a830d0caa509576f0e503818ee82c181b3437a"
+
 [[package]]
 name = "heck"
 version = "0.3.3"
@@ -999,7 +1018,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "bd070e393353796e801d209ad339e89596eb4c8d430d18ede6a1cced8fafbd99"
 dependencies = [
  "autocfg",
- "hashbrown",
+ "hashbrown 0.12.3",
 ]
 
 [[package]]
@@ -2050,24 +2069,27 @@ dependencies = [
 
 [[package]]
 name = "serial_test"
-version = "0.5.1"
+version = "2.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e0bccbcf40c8938196944a3da0e133e031a33f4d6b72db3bda3cc556e361905d"
+checksum = "0e56dd856803e253c8f298af3f4d7eb0ae5e23a737252cd90bb4f3b435033b2d"
 dependencies = [
+ "dashmap",
+ "futures",
  "lazy_static",
- "parking_lot 0.11.2",
+ "log",
+ "parking_lot 0.12.1",
  "serial_test_derive",
 ]
 
 [[package]]
 name = "serial_test_derive"
-version = "0.5.1"
+version = "2.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b2acd6defeddb41eb60bb468f8825d0cfd0c2a76bc03bfd235b6a1dc4f6a1ad5"
+checksum = "91d129178576168c589c9ec973feedf7d3126c01ac2bf08795109aa35b69fb8f"
 dependencies = [
  "proc-macro2",
  "quote",
- "syn 1.0.109",
+ "syn 2.0.22",
 ]
 
 [[package]]

Cargo.toml

@@ -74,7 +74,7 @@ rand = "0.8.5"
 [dev-dependencies]
 assert_cmd = "1.0.3"
 predicates-core = "1.0.2"
-serial_test = "0.5.1"
+serial_test = "2.0.0"
 
 [patch.crates-io]
 modular-bitfield = { git = "https://github.com/mthom/modular-bitfield" }

src/atom_table.rs

@@ -37,42 +37,61 @@ impl From<bool> for Atom {
     }
 }
 
-#[cfg(test)]
-use std::cell::RefCell;
-
 const ATOM_TABLE_INIT_SIZE: usize = 1 << 16;
 const ATOM_TABLE_ALIGN: usize = 8;
 
 #[cfg(test)]
 thread_local! {
-    static ATOM_TABLE_BUF_BASE: RefCell<*const u8> = RefCell::new(ptr::null_mut());
+   static ATOM_TABLE_BUF_BASE: std::cell::RefCell<*const u8> = std::cell::RefCell::new(ptr::null_mut());
 }
 
 #[cfg(not(test))]
-static mut ATOM_TABLE_BUF_BASE: *const u8 = ptr::null_mut();
+static ATOM_TABLE_BUF_BASE: std::sync::atomic::AtomicPtr<u8> =
+    std::sync::atomic::AtomicPtr::new(ptr::null_mut());
 
+fn set_atom_tbl_buf_base(old_ptr: *const u8, new_ptr: *const u8) -> Result<(), *const u8> {
 #[cfg(test)]
-fn set_atom_tbl_buf_base(ptr: *const u8) {
+    {
     ATOM_TABLE_BUF_BASE.with(|atom_table_buf_base| {
-        *atom_table_buf_base.borrow_mut() = ptr;
-    });
+        let mut borrow = atom_table_buf_base.borrow_mut();
+            if *borrow != old_ptr {
+                Err(*borrow)
+            } else {
+                *borrow = new_ptr;
+                Ok(())
+            }
+        })?;
+    };
+    #[cfg(not(test))]
+    {
+        ATOM_TABLE_BUF_BASE
+            .compare_exchange(
+                old_ptr.cast_mut(),
+                new_ptr.cast_mut(),
+                std::sync::atomic::Ordering::Relaxed,
+                std::sync::atomic::Ordering::Relaxed,
+            )
+            .map_err(|ptr| ptr.cast_const())
+    }?;
+    Ok(())
 }
 
-#[cfg(test)]
 pub(crate) fn get_atom_tbl_buf_base() -> *const u8 {
+    #[cfg(test)]
+    {
     ATOM_TABLE_BUF_BASE.with(|atom_table_buf_base| *atom_table_buf_base.borrow())
 }
-
 #[cfg(not(test))]
-fn set_atom_tbl_buf_base(ptr: *const u8) {
-    unsafe {
-        ATOM_TABLE_BUF_BASE = ptr;
+    {
+        ATOM_TABLE_BUF_BASE.load(std::sync::atomic::Ordering::Relaxed)
     }
 }
 
-#[cfg(not(test))]
-pub(crate) fn get_atom_tbl_buf_base() -> *const u8 {
-    unsafe { ATOM_TABLE_BUF_BASE }
+#[test]
+#[should_panic(expected = "Overwriting atom table base pointer")]
+fn atomtable_is_not_concurrency_safe() {
+    let _table_a = AtomTable::new();
+    let _table_b = AtomTable::new();
 }
 
 impl RawBlockTraits for AtomTable {
@@ -239,8 +258,17 @@ pub struct AtomTable {
     pub table: IndexSet<Atom>,
 }
 
+#[cold]
+fn atom_table_base_pointer_mismatch(expected: *const u8, got: *const u8) -> ! {
+    assert_eq!(expected, got, "Overwriting atom table base pointer, expected old value to be {expected:p}, but found {got:p}");
+    unreachable!("This should only be called in a case of a mismatch as such the assert_eq should have failed!")
+}
+
 impl Drop for AtomTable {
     fn drop(&mut self) {
+        if let Err(got) = set_atom_tbl_buf_base(self.block.base, ptr::null()) {
+            atom_table_base_pointer_mismatch(self.block.base, got);
+        }
         self.block.deallocate();
     }
 }
@@ -248,13 +276,17 @@ impl Drop for AtomTable {
 impl AtomTable {
     #[inline]
     pub fn new() -> Self {
-        let table = Self {
-            block: RawBlock::new(),
-            table: IndexSet::new(),
-        };
+        let mut block = RawBlock::new();
 
-        set_atom_tbl_buf_base(table.block.base);
-        table
+        if let Err(got) = set_atom_tbl_buf_base(ptr::null(), block.base) {
+            block.deallocate();
+            atom_table_base_pointer_mismatch(ptr::null(), got);
+        }
+
+        Self {
+            block,
+            table: IndexSet::new(),
+        }
     }
 
     #[inline]
@@ -289,8 +321,11 @@ impl AtomTable {
                     ptr = self.block.alloc(size);
 
                     if ptr.is_null() {
+                        let old_base = self.block.base;
                         self.block.grow();
-                        set_atom_tbl_buf_base(self.block.base);
+                        if let Err(got) = set_atom_tbl_buf_base(old_base, self.block.base) {
+                            atom_table_base_pointer_mismatch(old_base, got);
+                        }
                     } else {
                         break;
                     }

tests/scryer/issues.rs

@@ -1,4 +1,5 @@
 use crate::helper::{load_module_test, run_top_level_test_no_args, run_top_level_test_with_args};
+use scryer_prolog::machine::Machine;
 use serial_test::serial;
 
 // issue #857
@@ -128,10 +129,12 @@ fn compound_goal() {
 // issue #815
 #[test]
 fn no_stutter() {
-    run_top_level_test_no_args("write(a), write(b), false.\n\
+    run_top_level_test_no_args(
+        "write(a), write(b), false.\n\
                                 halt.\n\
                                 ",
-                               "ab   false.\n")
+        "ab   false.\n",
+    )
 }
 
 /*
@@ -168,3 +171,16 @@ fn call_0() {
         "   error(existence_error(procedure,call/0),call/0).\n",
     );
 }
+
+// issue #1206
+#[serial]
+#[test]
+#[should_panic(expected = "Overwriting atom table base pointer")]
+fn atomtable_is_not_concurrency_safe() {
+    // this is basically the same test as scryer_prolog::atom_table::atomtable_is_not_concurrency_safe
+    // but for this integration test scryer_prolog is compiled with cfg!(not(test))  while for the unit test it is compiled with cfg!(test)
+    // as the atom table implementation differ between cfg!(test) and cfg!(not(test)) both test serve a pourpose
+    // Note: this integration test itself is compiled with cfg!(test) independent of scryer_prolog itself
+    let _machine_a = Machine::with_test_streams();
+    let _machine_b = Machine::with_test_streams();
+}