Prevent stale fs-store writes after lock cleanup
What the developer wrote
Authored by Elias Rohrer
Prevent stale fs-store writes after lock cleanup
Reserve write versions while holding the per-path lock map mutex so
cleanup cannot remove the version state between version allocation and
lock reference acquisition.
Add a regression test for the ordering invariant.
Co-Authored-By: HAL 9000
This finding was discovered by Project Loupe
What changed, and why it matters
This patch fixes a subtle race condition in rust-lightning's file-system persistence layer. Previously, the code could reserve a new write version number before it had safely grabbed the corresponding per-file lock. A background cleanup task could then delete the lock entry in between, leaving the version number associated with a lock that no longer exists. The fix grabs the lock map first, then reserves the version while still holding that map lock, and adds a regression test to prove the ordering is correct.
Review the fix for correctness under all concurrency paths, ensure clean_locks and any other callers respect the new invariant, and consider whether additional synchronization is needed elsewhere in the fs_store module. Run the new regression test and related persistence tests.
Security signals we found
Race condition between version reservation and lock reference acquisition
Potential stale lock entry after lock-map cleanup
Concurrency bug in filesystem persistence store
Regression test added for ordering invariant
Evidence from the diff
In FilesystemStoreState::get_new_version_and_lock_ref, the old code called fetch_add to reserve a monotonic version, then called self.inner.get_inner_lock_ref(dest_file_path) to obtain/clone the Arc
Changed components
lightning-persister/src/fs_store/common.rsFilesystemStoreState::get_new_version_and_lock_refFilesystemStoreState::inner.locks mapFilesystemStoreState::clean_locksInspect captured patch +42 / −2
diff --git a/lightning-persister/src/fs_store/common.rs b/lightning-persister/src/fs_store/common.rs
index 885f806..5c73ad1 100644
--- a/lightning-persister/src/fs_store/common.rs
+++ b/lightning-persister/src/fs_store/common.rs
@@ -91,14 +91,17 @@ impl FilesystemStoreState {
}
fn get_new_version_and_lock_ref(&self, dest_file_path: PathBuf) -> (Arc<RwLock<u64>>, u64) {
+ let mut outer_lock = self.inner.locks.lock().unwrap();
+
let version = self.next_version.fetch_add(1, Ordering::Relaxed);
if version == u64::MAX {
panic!("FilesystemStore version counter overflowed");
}
// Get a reference to the inner lock. We do this early so that the arc can double as an in-flight counter for
- // cleaning up unused locks.
- let inner_lock_ref = self.inner.get_inner_lock_ref(dest_file_path);
+ // cleaning up unused locks. Allocate the version while holding the lock map mutex so that clean_locks cannot
+ // remove the entry after a version has been reserved but before its lock reference is cloned.
+ let inner_lock_ref = Arc::clone(&outer_lock.entry(dest_file_path).or_default());
(inner_lock_ref, version)
}
@@ -851,3 +854,40 @@ pub(crate) fn get_key_from_dir_entry_path(
},
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use std::sync::Arc;
+ use std::thread;
+ use std::time::Duration;
+
+ #[test]
+ fn version_is_not_reserved_before_lock_ref() {
+ let mut temp_path = std::env::temp_dir();
+ temp_path.push("test_version_is_not_reserved_before_lock_ref");
+ let state = Arc::new(FilesystemStoreState::new(temp_path));
+ let path =
+ state.get_checked_dest_file_path("ns", "sub", Some("key"), "write", false).unwrap();
+
+ let outer_lock = state.inner.locks.lock().unwrap();
+ let state_for_thread = Arc::clone(&state);
+ let path_for_thread = path.clone();
+ let handle =
+ thread::spawn(move || state_for_thread.get_new_version_and_lock_ref(path_for_thread));
+
+ thread::sleep(Duration::from_millis(50));
+ assert_eq!(
+ state.next_version.load(Ordering::Relaxed),
+ 1,
+ "version allocation must wait until the lock reference can be cloned"
+ );
+
+ drop(outer_lock);
+
+ let (inner_lock_ref, version) = handle.join().unwrap();
+ assert_eq!(version, 1);
+ state.inner.clean_locks(&inner_lock_ref, path);
+ }
+}
Why this scored 59/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.