wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
6.32k stars 1.79k forks source link

wxFileConfig unnecessarily does unlink() when writing config file on Linux #25088

Closed Safari77 closed 2 weeks ago

Safari77 commented 3 weeks ago

I didn't find config option to configure unlink-usage, but this app does just Write() and Flush() . Filesystem is ZFS.

1901923 14:12:20.465356 newfstatat(AT_FDCWD, "/home/safari/.config/spek/preferences", {st_dev=makedev(0, 0x25), st_ino=652836, st_mode=S_IFREG|0664, st_nlink=1, st_uid=500, st_gid=500, st_blksize=512, st_blocks=2, st_size=32, st_atime=1737114004 /* 2025-01-17T13:40:04.637207733+0200 */, st_atime_nsec=637207733, st_mtime=1737113992 /* 2025-01-17T13:39:52.971681859+0200 */, st_mtime_nsec=971681859, st_ctime=1737113992 /* 2025-01-17T13:39:52.971681859+0200 */, st_ctime_nsec=971681859}, 0) = 0 <0.000003>
1901923 14:12:20.465383 unlink("/home/safari/.config/spek/preferences") = 0 <0.000033>
1901923 14:12:20.465428 rename("/home/safari/.config/spek/preferencesrHwLVJ", "/home/safari/.config/spek/preferences") = 0 <0.000016>

rename overwrites the config file atomically (preferences), unlink it harmful: if crash happens after unlink and before rename, file contents are maybe gone, at least in theory.

wxGTK-3.2.6 on Fedora x86_64 Linux, Gnome 47 Wayland gtk-3.24.43

vadz commented 3 weeks ago

Thanks for reporting this, it indeed looks like we should indeed just remove these lines

https://github.com/wxWidgets/wxWidgets/blob/96418a86a17268276695a223339f6abd7de7f22a/src/common/file.cpp#L590-L593

I'll do it because the current code indeed looks like a good way to lose the file contents entirely...

Safari77 commented 3 weeks ago

On Windows the destination should be removed first.

And fflush / fsync before fclose :) (mingw needs a bit different handling for fsync) (... and check for EINTR; and EINVAL (which is okay to ignore if writing to some special devices)).

vadz commented 3 weeks ago

Oops, yes, MoveFile() doesn't remove the existing file without a special flag not used by rename(). And maybe we should actually use ReplaceFile() instead?

I don't think we want to add fsync here, this is a whole different can of worms.

Safari77 commented 3 weeks ago

Yeah, I add fsync into my own wxGTK build :)

Safari77 commented 3 weeks ago

wxFile::Flush already has HAVE_FSYNC check but it does not work when writing config files?

Safari77 commented 3 weeks ago

Note that fsync is not very scary, glib2 has had it since v2.66 in g_file_set_contents_full, which is often used to write config files. Without fsync the written file usually is zero bytes after a crash (when using a temporary file like now in wxGTK).

vadz commented 3 weeks ago

I think the latest PR version should work, please let me know if you still see any problems with it (not related to fsync).