Revert "Prevent stale fs-store writes after lock cleanup"
What changed, and why it matters
This commit undoes a previous safety change in the file-based storage component of the Lightning Dev Kit. The original change tried to prevent an old, slow write from overwriting a newer write after internal cleanup of unused locks. The revert argues the storage API never promised ordering during a write anyway, so the fix was unnecessary and added complexity. The revert removes a test that demonstrated the original problem, but does not itself introduce a clearly exploitable bug.
Review whether the reverted race window is reachable in production and whether the KVStore contract documentation accurately reflects the ordering behavior. If stale writes are possible, consider reinstating a narrower fix or adding a non-brittle regression test. Monitor for follow-up commits or maintainer discussion.
Security signals we found
Reverts a synchronization change intended to prevent stale writes from overwriting newer data
Removes regression test for stale-write-after-lock-cleanup scenario
Reintroduces a window between version allocation and lock-reference acquisition
No explicit security framing by the vendor in commit message or diff
Evidence from the diff
The commit reverts a prior patch that held the locks-map mutex while allocating a new version number, ensuring clean_locks could not remove a lock entry between version allocation and lock-reference cloning. After the revert, version allocation is again performed outside the locks-map mutex, and the lock reference is fetched separately via get_inner_lock_ref. The reverted code also removed a test-only hook and a regression test that simulated a stale write overtaking a newer write after lock cleanup. The commit message states the KVStore API cannot provide ordering guarantees inside write methods, so the synchronization was considered unnecessary and the test brittle.
Changed components
lightning-persister/src/fs_store/common.rsFilesystemStoreState::get_new_version_and_lock_refFilesystemStoreInner lock cleanup logicInspect captured patch +1 / −87
diff --git a/lightning-persister/src/fs_store/common.rs b/lightning-persister/src/fs_store/common.rs
index 96e5894..885f806 100644
--- a/lightning-persister/src/fs_store/common.rs
+++ b/lightning-persister/src/fs_store/common.rs
@@ -11,11 +11,7 @@ use std::collections::HashMap;
use std::fs;
use std::io::{ErrorKind, Read, Write};
use std::path::{Path, PathBuf};
-#[cfg(test)]
-use std::sync::atomic::AtomicBool;
use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
-#[cfg(test)]
-use std::sync::mpsc;
use std::sync::{Arc, Mutex, RwLock};
#[cfg(target_os = "windows")]
@@ -95,20 +91,14 @@ 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();
-
- // 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 version = self.next_version.fetch_add(1, Ordering::Relaxed);
if version == u64::MAX {
panic!("FilesystemStore version counter overflowed");
}
- #[cfg(test)]
- maybe_pause_after_version_allocation(&self.inner, &dest_file_path);
// 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 = Arc::clone(&outer_lock.entry(dest_file_path).or_default());
+ let inner_lock_ref = self.inner.get_inner_lock_ref(dest_file_path);
(inner_lock_ref, version)
}
@@ -861,79 +851,3 @@ pub(crate) fn get_key_from_dir_entry_path(
},
}
}
-
-#[cfg(test)]
-struct VersionAllocatedHook {
- dest_file_path: PathBuf,
- version_allocated: mpsc::Sender<()>,
- continue_write: Mutex<mpsc::Receiver<()>>,
- fired: AtomicBool,
-}
-
-#[cfg(test)]
-static VERSION_ALLOCATED_HOOK: Mutex<Option<Arc<VersionAllocatedHook>>> = Mutex::new(None);
-
-#[cfg(test)]
-fn maybe_pause_after_version_allocation(inner: &FilesystemStoreInner, dest_file_path: &Path) {
- let hook = VERSION_ALLOCATED_HOOK.lock().unwrap().clone();
- if let Some(hook) = hook {
- if hook.dest_file_path.as_path() != dest_file_path
- || hook.fired.swap(true, Ordering::AcqRel)
- {
- return;
- }
-
- let version_allocation_holds_lock = inner.locks.try_lock().is_err();
- hook.version_allocated.send(()).unwrap();
- if !version_allocation_holds_lock {
- hook.continue_write.lock().unwrap().recv().unwrap();
- }
- }
-}
-
-#[cfg(test)]
-mod tests {
- use super::*;
-
- use std::sync::Arc;
- use std::thread;
-
- #[test]
- fn stale_write_after_lock_cleanup_does_not_overwrite_newer_write() {
- let mut temp_path = std::env::temp_dir();
- temp_path.push("test_stale_write_after_lock_cleanup");
- let _ = std::fs::remove_dir_all(&temp_path);
-
- let state = Arc::new(FilesystemStoreState::new(temp_path.clone()));
- let path =
- state.get_checked_dest_file_path("ns", "sub", Some("key"), "write", false).unwrap();
- let (version_allocated, wait_for_version) = mpsc::channel();
- let (continue_write, wait_to_continue) = mpsc::channel();
- *VERSION_ALLOCATED_HOOK.lock().unwrap() = Some(Arc::new(VersionAllocatedHook {
- dest_file_path: path.clone(),
- version_allocated,
- continue_write: Mutex::new(wait_to_continue),
- fired: AtomicBool::new(false),
- }));
-
- let state_for_thread = Arc::clone(&state);
- let path_for_thread = path.clone();
- let stale_write = thread::spawn(move || {
- let (inner_lock_ref, version) =
- state_for_thread.get_new_version_and_lock_ref(path_for_thread.clone());
- state_for_thread
- .inner
- .write_version(inner_lock_ref, path_for_thread, b"stale".to_vec(), version, false)
- .unwrap();
- });
-
- wait_for_version.recv().unwrap();
- state.write_impl("ns", "sub", "key", b"newer".to_vec(), false).unwrap();
- continue_write.send(()).unwrap();
- stale_write.join().unwrap();
- *VERSION_ALLOCATED_HOOK.lock().unwrap() = None;
-
- assert_eq!(state.read_impl("ns", "sub", "key", false).unwrap(), b"newer");
- let _ = std::fs::remove_dir_all(temp_path);
- }
-}
Why this scored 32/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.