[vim/vim] Undo feature ignores fsync setting and writes undofile in an unsafe manner (#8879)

[Sami Farin]

If undofile is set, vim writes the undofile in an unsafe manner:

17:24:45.814796 unlink("/home/safari/.vim/undo/%home%safari%undotest.txt") = 0 <0.000096>
17:24:45.814926 openat(AT_FDCWD, "/home/safari/.vim/undo/%home%safari%undotest.txt", O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0600) = 4 <0.000033>
17:24:45.815260 write(4, "Vim\237U"..., 1802) = 1802 <0.000022>
17:24:45.815329 close(4)                = 0 <0.000017>

Describe the bug
If there is a problem (crash, power outage), undofile is in inconsistent state or 0 bytes.
How to write a file in a safe manner has been known for some decades, but here I could say: temporary file, fsync, rename.

To Reproduce
Use undofile feature with or without fsync flags.

Expected behavior
Write file safely.

Environment (please complete the following information):

• 8.2.3435
• OS: Fedora 33

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.

[Bram Moolenaar]

Well, the undo file is not essential, if it goes lost then you still have the original text. But it's true that you may depend on undo working, e.g. when making some temporary changes that you will undo later. Still, I don't consider making this more reliable a high priority.

[Christian Brabandt]

I had a real quick look and used this:

diff --git a/src/undo.c b/src/undo.c
index 814130108..5fed2dede 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -1786,6 +1786,11 @@ u_write_undo(
        write_ok = FALSE;
 #endif

+#if defined(UNIX) && defined(HAVE_FSYNC)
+       if (p_fs && fd > 0 && fflush(fp) == 0 && vim_fsync(fd) != 0)
+           write_ok = FALSE;
+#endif
+
 write_error:
     fclose(fp);
     if (!write_ok)

That seems to be doing it according to strace output:

  unlink("/home/chrisbra/code/vim-src/src/.fsync.txt.un~") = 0
  openat(AT_FDCWD, "/home/chrisbra/code/vim-src/src/.fsync.txt.un~", O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0644) = 3
  chmod("/home/chrisbra/code/vim-src/src/.fsync.txt.un~", 0644) = 0
  stat("/home/chrisbra/code/vim-src/src/.fsync.txt.un~", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
  fcntl(3, F_GETFL)                       = 0x28001 (flags O_WRONLY|O_LARGEFILE|O_NOFOLLOW)
  fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
  write(3, "Vim\237UnDo\345\0\2\261\260S\375\223\6\242\221\223\32\377\257\322M\215\257\23\274\270AG"..., 4096) = 4096
  write(3, "\0\0\0\6\0\0\0\n\0\0\0\10\0\0\0\r\0\0\0\4\0\0\0\0\377\377\377\377\0\1\0\0"..., 1815) = 1815
  fsync(3)                                = 0
  close(3)                                = 0

[Bram Moolenaar]

The patch flushes "fd" but the writing is on "fp".

[Bram Moolenaar]

using fileno() should probably work

[vim-dev ML]

On Mo, 27 Sep 2021, Bram Moolenaar wrote:

> The patch flushes "fd" but the writing is on "fp".

Yes, it needs to flush the FILE stream pointer before we can sync the
underlying file descriptor. Otherwise the fsync could happen too early.

I don't think we need fileno(), as the filedescriptor fd is still
available (from when opening the stream, around undo.c:1721)

Best,
Christian
--
Das Leben wird gegen Abend, wie die Träume gegen Morgen, immer klarer.
-- Karl Julius Weber

[Bram Moolenaar]

Closed #8879 via 340dd0f.

—
You are receiving this because you commented.

[Sami Farin]

Adding fsync is better, but it still does not use a temporary file and rename to be 100% safe.

—
You are receiving this because you commented.