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

Skip to first unread message

Sami Farin

Sep 16, 2021, 10:51:48 AMSep 16
to vim/vim, Subscribed

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

Sep 22, 2021, 8:52:20 AMSep 22
to vim/vim, Subscribed

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

Sep 22, 2021, 12:35:32 PMSep 22
to vim/vim, Subscribed

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;

+#if defined(UNIX) && defined(HAVE_FSYNC)
+       if (p_fs && fd > 0 && fflush(fp) == 0 && vim_fsync(fd) != 0)
+           write_ok = FALSE;
     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

Sep 27, 2021, 4:36:55 PMSep 27
to vim/vim, Subscribed

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

Bram Moolenaar

Sep 27, 2021, 4:38:33 PMSep 27
to vim/vim, Subscribed

using fileno() should probably work

vim-dev ML

Sep 27, 2021, 4:57:56 PMSep 27
to vim/vim, vim-dev ML, Your activity

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)

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

Bram Moolenaar

Oct 14, 2021, 12:52:50 PM (5 days ago) Oct 14
to vim/vim, vim-dev ML, Comment

Closed #8879 via 340dd0f.

You are receiving this because you commented.

Sami Farin

Oct 14, 2021, 3:55:38 PM (5 days ago) Oct 14
to vim/vim, vim-dev ML, Comment

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.

Reply all
Reply to author
0 new messages