From 984090179521cfac0aafe9c1502db9f0d87ad895 Mon Sep 17 00:00:00 2001 From: Christian Eltzschig Date: Mon, 15 Jan 2024 03:29:29 +0100 Subject: [PATCH 1/2] [#87] Fix undefined behavior in spsc {queue|index_queue} --- doc/release-notes/iceoryx2-unreleased.md | 1 + iceoryx2-bb/lock-free/src/spsc/index_queue.rs | 3 +++ iceoryx2-bb/lock-free/src/spsc/queue.rs | 5 ++++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/release-notes/iceoryx2-unreleased.md b/doc/release-notes/iceoryx2-unreleased.md index cbcf1cec8..bfe62c850 100644 --- a/doc/release-notes/iceoryx2-unreleased.md +++ b/doc/release-notes/iceoryx2-unreleased.md @@ -16,6 +16,7 @@ * Fix `clock_nanosleep` on macOS [#80](https://github.com/eclipse-iceoryx/iceoryx2/issues/80) * Fix broken `sighandler_t` translation [#81](https://github.com/eclipse-iceoryx/iceoryx2/issues/81) + * Fix undefined behavior in `spsc::{queue|index_queue}` [#87](https://github.com/eclipse-iceoryx/iceoryx2/issues/87) ### Refactoring diff --git a/iceoryx2-bb/lock-free/src/spsc/index_queue.rs b/iceoryx2-bb/lock-free/src/spsc/index_queue.rs index 37fe2c130..c19da1bae 100644 --- a/iceoryx2-bb/lock-free/src/spsc/index_queue.rs +++ b/iceoryx2-bb/lock-free/src/spsc/index_queue.rs @@ -316,6 +316,9 @@ pub mod details { } let value = unsafe { *self.at(read_position) }; + // prevent that `out` and `read_position` statements are reordered according to + // the AS-IF rule. + core::sync::atomic::fence(Ordering::SeqCst); self.read_position .store(read_position + 1, Ordering::Relaxed); diff --git a/iceoryx2-bb/lock-free/src/spsc/queue.rs b/iceoryx2-bb/lock-free/src/spsc/queue.rs index 7375889f0..650fb17c3 100644 --- a/iceoryx2-bb/lock-free/src/spsc/queue.rs +++ b/iceoryx2-bb/lock-free/src/spsc/queue.rs @@ -215,8 +215,11 @@ impl Queue { .as_ptr() }; + // prevent that `out` and `read_position` statements are reordered according to + // the AS-IF rule. + core::sync::atomic::fence(Ordering::SeqCst); self.read_position - .store(current_read_pos + 1, Ordering::Release); + .store(current_read_pos + 1, Ordering::Relaxed); Some(out) } From 287cb1652e1a2eb57b3b1c59fdd7fee362c3ec80 Mon Sep 17 00:00:00 2001 From: Christian Eltzschig Date: Tue, 16 Jan 2024 23:23:57 +0100 Subject: [PATCH 2/2] [#87] SoFi push-overflow from different threads, non-concurrently; atomic fence in fifo with AcqRel to prevent reordering --- iceoryx2-bb/lock-free/src/spsc/index_queue.rs | 2 +- iceoryx2-bb/lock-free/src/spsc/queue.rs | 2 +- .../lock-free/src/spsc/safely_overflowing_index_queue.rs | 7 ++++++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/iceoryx2-bb/lock-free/src/spsc/index_queue.rs b/iceoryx2-bb/lock-free/src/spsc/index_queue.rs index c19da1bae..60ab23406 100644 --- a/iceoryx2-bb/lock-free/src/spsc/index_queue.rs +++ b/iceoryx2-bb/lock-free/src/spsc/index_queue.rs @@ -318,7 +318,7 @@ pub mod details { let value = unsafe { *self.at(read_position) }; // prevent that `out` and `read_position` statements are reordered according to // the AS-IF rule. - core::sync::atomic::fence(Ordering::SeqCst); + core::sync::atomic::fence(Ordering::AcqRel); self.read_position .store(read_position + 1, Ordering::Relaxed); diff --git a/iceoryx2-bb/lock-free/src/spsc/queue.rs b/iceoryx2-bb/lock-free/src/spsc/queue.rs index 650fb17c3..0a7fba882 100644 --- a/iceoryx2-bb/lock-free/src/spsc/queue.rs +++ b/iceoryx2-bb/lock-free/src/spsc/queue.rs @@ -217,7 +217,7 @@ impl Queue { // prevent that `out` and `read_position` statements are reordered according to // the AS-IF rule. - core::sync::atomic::fence(Ordering::SeqCst); + core::sync::atomic::fence(Ordering::AcqRel); self.read_position .store(current_read_pos + 1, Ordering::Relaxed); diff --git a/iceoryx2-bb/lock-free/src/spsc/safely_overflowing_index_queue.rs b/iceoryx2-bb/lock-free/src/spsc/safely_overflowing_index_queue.rs index d49c7b9ca..586d272ee 100644 --- a/iceoryx2-bb/lock-free/src/spsc/safely_overflowing_index_queue.rs +++ b/iceoryx2-bb/lock-free/src/spsc/safely_overflowing_index_queue.rs @@ -302,7 +302,12 @@ pub mod details { /// * It has to be ensured that the memory is initialized with /// [`SafelyOverflowingIndexQueue::init()`]. pub unsafe fn push(&self, value: usize) -> Option { - let write_position = self.write_position.load(Ordering::Relaxed); + //////////////// + // SYNC POINT R + //////////////// + // required when push in overflow case is called non-concurrently from a different + // thread + let write_position = self.write_position.load(Ordering::Acquire); let read_position = self.read_position.load(Ordering::Relaxed); let is_full = write_position == read_position + self.capacity;