Avoid leaking stale filesystem store temp files#4676
Conversation
Async filesystem writes may be awaited out of order, making older writes stale after their temporary data has already been created. Ensure those stale or failed write attempts do not leave historical plaintext data in *.tmp files. Co-Authored-By: HAL 9000 This finding was discovered by Project Loupe
|
👋 Thanks for assigning @benthecarman as a reviewer! |
| if tmp_file_needs_cleanup { | ||
| if let Err(e) = remove_file_if_exists(&tmp_file_path) { | ||
| if write_res.is_ok() { | ||
| return Err(e); | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| write_res |
There was a problem hiding this comment.
When the write is stale (a newer version already won, so execute_locked_write skipped the callback and returned Ok(())), tmp_file_needs_cleanup is still true. If remove_file_if_exists then fails, this converts an otherwise-successful, consistent no-op write into an Err. The actual key data on disk is correct (the newer version persisted it), so the caller now sees a persistence failure for a write that effectively succeeded, which in LDK can trigger unnecessary error handling/retries.
Consider whether a failed temp-file cleanup should really fail the call in the stale case, or just be logged/ignored (as is done in the pre-lock failure path on line 304 with let _ = ...).
|
I've re-examined the full diff and surrounding code. The closure mutating Summary No new issues found. Previously posted (still applicable):
|
| if tmp_file_needs_cleanup { | ||
| if let Err(e) = remove_file_if_exists(&tmp_file_path) { | ||
| if write_res.is_ok() { | ||
| return Err(e); |
There was a problem hiding this comment.
agree with claude here, i think this if should be if write_res.is_err(). If we successfully persist but dont remove the file, its better to just continue on (list will clean it up eventually). However, also this seems like an impossible path to get to
tnull
force-pushed
the
2026-06-fs-store-stale-tmp-cleanup
branch
from
ffe263c to
9de63a0
Compare
|
Updated to line-wrap commit messages. |
4 participants
Async filesystem writes may be awaited out of order, making older writes stale after their temporary data has already been created. Ensure those stale or failed write attempts do not leave historical plaintext data in *.tmp files.
Co-Authored-By: HAL 9000
This finding was discovered by Project Loupe