marlam / mpop

POP3 client
https://marlam.de/mpop
GNU General Public License v3.0
13 stars 1 forks source link

Crash consistency of maildir delivery? #8

Closed Artefact2 closed 3 years ago

Artefact2 commented 3 years ago

Hello,

I am reading the source here https://github.com/marlam/mpop-mirror/blob/master/src/delivery.c#L389-L423 and am wondering if it is possible to lose e-mails after a crash (very inconvenient especially if the server doesn't keep messages).

As I understand it, mpop writes the received e-mail in a temp file, fsync()s it, link()s it to its final pathname then unlink()s the temp file. Once mpop is done, the link()/unlink() calls have yet to hit the disk and the filesystem can reorder them. So it's possible that, after a crash, unlink() was persisted to disk, but not link(), resulting in a lost e-mail.

To my understanding, POSIX does not guarantee ordering of such operations, and many filesystems don't either.

One fix would be to fsync() the newly linked file and its directory entry before unlinking, like below:

diff --git a/src/delivery.c b/src/delivery.c
index c4acb62..09d7a0b 100644
--- a/src/delivery.c
+++ b/src/delivery.c
@@ -390,6 +390,7 @@ int delivery_method_maildir_close(delivery_method_t *dm, char **errstr)
 {
     maildir_data_t *maildir_data;
     char *newfilename;
+    int newfd;

     maildir_data = dm->data;
     if (fsync(fileno(dm->pipe)) != 0)
@@ -414,6 +415,32 @@ int delivery_method_maildir_close(delivery_method_t *dm, char **errstr)
         free(newfilename);
         return DELIVERY_EIO;
     }
+    if ((newfd = open(newfilename, O_RDONLY)) < 0) {
+        *errstr = xasprintf(_("cannot open %s%c%s: %s"), maildir_data->maildir,
+                            PATH_SEP, newfilename, strerror(errno));
+        free(newfilename);
+        return DELIVERY_EIO;
+    }
+    if (fsync(newfd) != 0) {
+        *errstr = xasprintf(_("cannot sync %s%c%s: %s"), maildir_data->maildir,
+                            PATH_SEP, newfilename, strerror(errno));
+        free(newfilename);
+        return DELIVERY_EIO;
+    }
+    close(newfd);
+    if ((newfd = open(maildir_data->maildir, O_RDONLY)) < 0) {
+        *errstr = xasprintf(_("cannot open %s: %s"), maildir_data->maildir,
+                  strerror(errno));
+        free(newfilename);
+        return DELIVERY_EIO;
+    }
+    if (fsync(newfd) != 0) {
+        *errstr = xasprintf(_("cannot sync %s: %s"), maildir_data->maildir,
+                  strerror(errno));
+        free(newfilename);
+        return DELIVERY_EIO;
+    }
+    close(newfd);
     (void)unlink(maildir_data->filename);
     free(newfilename);
     free(maildir_data->filename);
marlam commented 3 years ago

I don't see a problem with link()/unlink(). That is the standard way to deliver mail to maildir folders, and I doubt that a file system can reorder operations in the way you describe. Please have a look at the maildir documentation and the Wikipedia article on maildir.

Artefact2 commented 3 years ago

I don't see any mention in those two links that you have to use link/unlink. Filesystems can and do reorder a lot of writes especially metadata (that's why write(tmpfile) rename(tmpfile,a) can leave you with a truncated a and no tmpfile).

Why not use rename(), which is guaranteed to be atomic?

marlam commented 3 years ago

Yes, you can use rename() too. But you don't have to, since maildir does not require the link()/unlink() pair to be atomic.