From: Kevin McCarthy <kevin@8t8.us>
Date: Mon, 27 Aug 2018 01:43:20 +0000 (-0700)
Subject: Add additional error handling to safe_rename().
X-Git-Tag: mutt-1-11-rel~76
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2d88922756c9c72d5530308383227851c21160fb;p=mutt

Add additional error handling to safe_rename().

It is apparently possible for link() to return an error but the link
to still be created.  Add a double check for that case.  If the files
match, unlink the src and return success.
---

diff --git a/lib.c b/lib.c
index 9a5d5325..1c8b8682 100644
--- a/lib.c
+++ b/lib.c
@@ -446,12 +446,37 @@ int safe_symlink(const char *oldpath, const char *newpath)
 int safe_rename (const char *src, const char *target)
 {
   struct stat ssb, tsb;
+  int link_errno;
 
   if (!src || !target)
     return -1;
 
   if (link (src, target) != 0)
   {
+    link_errno = errno;
+
+    /*
+     * It is historically documented that link can return -1 if NFS
+     * dies after creating the link.  In that case, we are supposed
+     * to use stat to check if the link was created.
+     *
+     * Derek Martin notes that some implementations of link() follow a
+     * source symlink.  It might be more correct to use stat() on src.
+     * I am not doing so to minimize changes in behavior: the function
+     * used lstat() further below for 20 years without issue, and I
+     * believe was never intended to be used on a src symlink.
+     */
+    if ((lstat (src, &ssb) == 0) &&
+        (lstat (target, &tsb) == 0) &&
+        (compare_stat (&ssb, &tsb) == 0))
+    {
+      dprint (1, (debugfile,
+                  "safe_rename: link (%s, %s) reported failure: %s (%d) but actually succeded\n",
+                  src, target, strerror (errno), errno));
+      goto success;
+    }
+
+    errno = link_errno;
 
     /*
      * Coda does not allow cross-directory links, but tells
@@ -533,11 +558,11 @@ int safe_rename (const char *src, const char *target)
   }
 #endif
 
+success:
   /*
    * Unlink the original link.  Should we really ignore the return
    * value here? XXX
    */
-
   if (unlink (src) == -1) 
   {
     dprint (1, (debugfile, "safe_rename: unlink (%s) failed: %s (%d)\n",