From bc9f0732e77042dd640d319614794b4c0bfabcbd Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 5 Nov 2023 11:14:15 +0100 Subject: [PATCH 1/2] Provide safety contract for `unlock` --- polkadot/node/tracking-allocator/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/polkadot/node/tracking-allocator/src/lib.rs b/polkadot/node/tracking-allocator/src/lib.rs index 8441bd1ffafa..0a9029ce9fd9 100644 --- a/polkadot/node/tracking-allocator/src/lib.rs +++ b/polkadot/node/tracking-allocator/src/lib.rs @@ -72,8 +72,10 @@ impl Spinlock { } } + // SAFETY: It should be only called from the guard's destructor. Calling it explicitly while + // the guard is alive is undefined behavior. #[inline] - fn unlock(&self) { + unsafe fn unlock(&self) { self.lock.store(false, Ordering::Release); } } @@ -97,7 +99,9 @@ impl DerefMut for SpinlockGuard<'_, T> { impl Drop for SpinlockGuard<'_, T> { fn drop(&mut self) { - self.lock.unlock(); + // SAFETY: Calling `unlock` is only safe when it's guaranteed no guard outlives the + // unlocking point; here, the guard is dropped, so it is safe. + unsafe { self.lock.unlock() } } } From b6cc05560d38c7ea75d5f93e7fd932fb884b7be0 Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 5 Nov 2023 13:12:52 +0100 Subject: [PATCH 2/2] Improve comment --- polkadot/node/tracking-allocator/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/polkadot/node/tracking-allocator/src/lib.rs b/polkadot/node/tracking-allocator/src/lib.rs index 0a9029ce9fd9..ab8597b5c382 100644 --- a/polkadot/node/tracking-allocator/src/lib.rs +++ b/polkadot/node/tracking-allocator/src/lib.rs @@ -73,7 +73,8 @@ impl Spinlock { } // SAFETY: It should be only called from the guard's destructor. Calling it explicitly while - // the guard is alive is undefined behavior. + // the guard is alive is undefined behavior, as it breaks the security contract of `Deref` and + // `DerefMut`, which implies that lock is held at the moment of dereferencing. #[inline] unsafe fn unlock(&self) { self.lock.store(false, Ordering::Release);