matthiasbeyer
commented
3 years ago Thank you for your report.
You're probably right, and you're definitively right about the open-file-then-serialize problem. I'd welcome a patch for this which reorders the steps to first-serialize-then-open-file.
About the other things you mentioned: I've never experienced such an issue, but I see where this is coming from. I'd love to see patches/improvement over the current state, although I'm not 100% sure whether the rename approach solves the issue (I have to read up on the fsync stuff you mentioned).
Zykino
nyanpasu64
joelthelion
Dylan-DPC-zz
I've noticed that when confy tries to write to a file path, it overwrites the file in-place:
https://github.com/rust-cli/confy/blob/cd6932214ec16d50aa0f0defeb847a2e12618e61/src/lib.rs#L281-L287
In my experience (though not with apps using confy), overwriting files in-place causes problems. One possible issue (for files shared between many apps) is that either other applications will be able to read half-written contents, or the file will be locked and unreadable for the duration of the write. A more serious integrity issue is that if the application terminates during the file write (either from panicking, or from being terminated in taskmgr or Ctrl-C'd), it will result in a corrupted file being written to disk, with both the old and new file data being lost.
As an example of what can go wrong, a Python app I wrote (corrscope) saves config files directly to disk (since I didn't know better at the time), overwriting the previous file. Every few months I get a report from a user who can't open the app because the config file is corrupt and fails to load.
What causes saving to fail?
Confy's
store_path()first opens the file, then calls eithertoml::to_string_prettyorserde_yaml::to_string(both of which are fallible and returnResult<...>), and use the?operator on theResult. If the function called fails, thenstore_path()exits, but the file on the disk has already been truncated and the damage is done. (I'm not sure if TOML or YAML serialization is likely to fail, either reliably or on edge cases, in unfinished programs on developer machines or in released applications on user machines.)In corrscope, I suspect a similar event can occur if the serializer is left in a corrupted state. (I use a persistent global YAML serializer object for both documents and settings, since I hadn't learned Rust at the time, which would've taught me to avoid shared mutability.)
On top of this, there's the risk of getting terminated or Ctrl-C'd during the serialization operation (or if the serialization hangs and the app needs to be killed). I'm not sure if a system crash after the app finishes writing can lead to corruption, in various filesystems and journaling modes.
Solutions
A common strategy is to write to a temporary file and rename it on top of the original. While imperfect (may not always preserve permissions or owners), it's the least bad strategy I'm aware of for saving generic textual config files. (An alternative, advocated by danluu, is to not rename on top of the original file, but use an external log file for crash recovery. However, this introduces complexity in the app, and is unworkable if other apps expect plaintext config files and don't understand how to recover from log files.)
Additionally if you want to recover from system crashes as well, you need to fsync the temporary file before renaming it to the original filename. If you don't do so, it's possible that the temporary file gets renamed and overwrites the original file (and the rename gets flushed to disk), but the system crashes before the temporary file's contents are written to disk.
Yet another level of protection is fsyncing the directory to ensure that if the system crashes after renaming the temporary file over the old file, the rename can't be reverted. However this is slower than not fsyncing the directory, is not necessary to prevent corruption on Linux, and is not possible on Windows.
https://github.com/untitaker/rust-atomicwrites is a crate that performs atomic file writing. However it performs a directory fsync on Linux, which I think is unnecessary, slow, and inconsistent between platforms. I would rewrite or vendor the crate without performing a directory fsync.