Avoid leaking stale filesystem store temp files
What the developer wrote
Authored by Elias Rohrer
Avoid leaking stale filesystem store temp files
Async filesystem writes may be awaited out of order, making older writes stale after their temporary data has already been created. Clean up stale or failed write attempts on a best-effort basis so they do not leave historical plaintext data in *.tmp files, without reporting cleanup failures as persistence failures.
Co-Authored-By: HAL 9000
This finding was discovered by Project Loupe
What changed, and why it matters
This change fixes a cleanup problem in the Lightning Dev Kit's file-saving code. When two saves for the same file happen at nearly the same time, an earlier, out-of-date save could leave behind a temporary file containing plain data. The patch now deletes those leftover temporary files so sensitive information isn't left sitting on disk longer than necessary. It is a defensive hardening fix rather than a direct remote exploit.
Apply the patch. Review backup and file-recovery procedures on nodes that may have run affected versions to ensure no historical .tmp files remain in the persistence directory. Consider adding monitoring or periodic cleanup for orphaned .tmp files on existing deployments until patched.
Security signals we found
Temporary file leak of historical plaintext data
Best-effort cleanup added on write failure and stale write paths
Async write ordering can produce stale temp files
Plaintext channel state potentially recoverable from leftover .tmp files
Defensive hardening in persistence layer
Evidence from the diff
The patch modifies FilesystemStoreInner::write in lightning-persister/src/fs_store/common.rs to remove temporary files when a write fails or when a newer write completes first and makes an older in-flight write stale. Previously, async writes awaited out of order could leave *.tmp files containing historical plaintext channel data. The fix wraps the temp-file creation and the locked destination write in cleanup logic that best-effort deletes the temp file on any error or stale outcome, without treating cleanup failures as persistence failures. A new test in v2.rs verifies that two concurrent writes to the same key leave no .tmp files behind.
Changed components
lightning-persister/src/fs_store/common.rslightning-persister/src/fs_store/v2.rsFilesystemStoreInner::writeFilesystemStoreV2Inspect captured patch +91 / −41
diff --git a/lightning-persister/src/fs_store/common.rs b/lightning-persister/src/fs_store/common.rs
index 885f806..b591e43 100644
--- a/lightning-persister/src/fs_store/common.rs
+++ b/lightning-persister/src/fs_store/common.rs
@@ -277,33 +277,43 @@ impl FilesystemStoreInner {
let tmp_file_ext = format!("{}.tmp", self.tmp_file_counter.fetch_add(1, Ordering::AcqRel));
tmp_file_path.set_extension(tmp_file_ext);
- {
- let mut tmp_file = fs::File::create(&tmp_file_path)?;
- tmp_file.write_all(&buf)?;
-
- // If we need to preserve the original mtime (for updates), set it before fsync.
- if let Some(mtime) = mtime {
- let times = fs::FileTimes::new().set_modified(mtime);
- tmp_file.set_times(times)?;
- }
+ let tmp_file_res = match fs::File::create(&tmp_file_path) {
+ Ok(mut tmp_file) => (|| -> lightning::io::Result<()> {
+ tmp_file.write_all(&buf)?;
+
+ // If we need to preserve the original mtime (for updates), set it before fsync.
+ if let Some(mtime) = mtime {
+ let times = fs::FileTimes::new().set_modified(mtime);
+ tmp_file.set_times(times)?;
+ }
- tmp_file.sync_all()?;
+ tmp_file.sync_all()?;
+ Ok(())
+ })(),
+ Err(e) => return Err(e.into()),
+ };
+ if let Err(e) = tmp_file_res {
+ let _ = fs::remove_file(&tmp_file_path);
+ return Err(e);
}
- self.execute_locked_write(inner_lock_ref, dest_file_path.clone(), version, || {
- #[cfg(not(target_os = "windows"))]
- {
- fs::rename(&tmp_file_path, &dest_file_path)?;
- let dir_file = fs::OpenOptions::new().read(true).open(&parent_directory)?;
- dir_file.sync_all()?;
- Ok(())
- }
+ let mut tmp_file_needs_cleanup = true;
+ let write_res =
+ self.execute_locked_write(inner_lock_ref, dest_file_path.clone(), version, || {
+ #[cfg(not(target_os = "windows"))]
+ {
+ fs::rename(&tmp_file_path, &dest_file_path)?;
+ tmp_file_needs_cleanup = false;
+ let dir_file = fs::OpenOptions::new().read(true).open(&parent_directory)?;
+ dir_file.sync_all()?;
+ Ok(())
+ }
- #[cfg(target_os = "windows")]
- {
- let res = if dest_file_path.exists() {
- call!(unsafe {
- windows_sys::Win32::Storage::FileSystem::ReplaceFileW(
+ #[cfg(target_os = "windows")]
+ {
+ let res = if dest_file_path.exists() {
+ call!(unsafe {
+ windows_sys::Win32::Storage::FileSystem::ReplaceFileW(
path_to_windows_str(&dest_file_path).as_ptr(),
path_to_windows_str(&tmp_file_path).as_ptr(),
std::ptr::null(),
@@ -311,30 +321,37 @@ impl FilesystemStoreInner {
std::ptr::null_mut() as *const core::ffi::c_void,
std::ptr::null_mut() as *const core::ffi::c_void,
)
- })
- } else {
- call!(unsafe {
- windows_sys::Win32::Storage::FileSystem::MoveFileExW(
+ })
+ } else {
+ call!(unsafe {
+ windows_sys::Win32::Storage::FileSystem::MoveFileExW(
path_to_windows_str(&tmp_file_path).as_ptr(),
path_to_windows_str(&dest_file_path).as_ptr(),
windows_sys::Win32::Storage::FileSystem::MOVEFILE_WRITE_THROUGH
| windows_sys::Win32::Storage::FileSystem::MOVEFILE_REPLACE_EXISTING,
)
- })
- };
-
- match res {
- Ok(()) => {
- // We fsync the dest file in hopes this will also flush the metadata to disk.
- let dest_file =
- fs::OpenOptions::new().read(true).write(true).open(&dest_file_path)?;
- dest_file.sync_all()?;
- Ok(())
- },
- Err(e) => Err(e.into()),
+ })
+ };
+
+ match res {
+ Ok(()) => {
+ tmp_file_needs_cleanup = false;
+ // We fsync the dest file in hopes this will also flush the metadata to disk.
+ let dest_file = fs::OpenOptions::new()
+ .read(true)
+ .write(true)
+ .open(&dest_file_path)?;
+ dest_file.sync_all()?;
+ Ok(())
+ },
+ Err(e) => Err(e.into()),
+ }
}
- }
- })
+ });
+ if tmp_file_needs_cleanup {
+ let _ = fs::remove_file(&tmp_file_path);
+ }
+ write_res
}
fn remove_version(
diff --git a/lightning-persister/src/fs_store/v2.rs b/lightning-persister/src/fs_store/v2.rs
index fe1fdf6..af0ad4f 100644
--- a/lightning-persister/src/fs_store/v2.rs
+++ b/lightning-persister/src/fs_store/v2.rs
@@ -444,6 +444,39 @@ mod tests {
assert_eq!(listed_keys.len(), 0);
}
+ #[cfg(feature = "tokio")]
+ #[tokio::test]
+ async fn stale_write_does_not_leak_tmp_file() {
+ use lightning::util::persist::KVStore;
+
+ let mut temp_path = std::env::temp_dir();
+ temp_path.push("test_stale_write_does_not_leak_tmp_file_v2");
+ let _ = fs::remove_dir_all(&temp_path);
+ let fs_store = FilesystemStoreV2::new(temp_path.clone()).unwrap();
+
+ let data1 = vec![1u8; 32];
+ let data2 = vec![2u8; 32];
+
+ let primary = "testspace";
+ let secondary = "testsubspace";
+ let key = "testkey";
+
+ let fut1 = KVStore::write(&fs_store, primary, secondary, key, data1);
+ let fut2 = KVStore::write(&fs_store, primary, secondary, key, data2);
+
+ fut2.await.unwrap();
+ fut1.await.unwrap();
+
+ let dir = temp_path.join(primary).join(secondary);
+ let tmp_files: Vec<_> = fs::read_dir(&dir)
+ .unwrap()
+ .filter_map(|e| e.ok())
+ .map(|e| e.path())
+ .filter(|p| p.extension().map_or(false, |ext| ext == "tmp"))
+ .collect();
+ assert!(tmp_files.is_empty(), "Found leaked tmp files: {:?}", tmp_files);
+ }
+
#[test]
fn test_data_migration() {
let mut source_temp_path = std::env::temp_dir();
Why this scored 50/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.