Re: bug#52410: "mv -T --backup=numbered hello world/" renameat2 infinite loop

Re: bug#52410: "mv -T --backup=numbered hello world/" renameat2 infinite loop

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On 12/10/21 02:22, Vincent Vermilya wrote:
> If DEST directory has a trailing slash in "mv -T --backup=numbered", mv
> gets stuck in an infinite loop. This does not happen without the trailing
> slash. strace shows renameat2 as the call that gets looped on.

Thanks for the bug report. This is due to a Gnulib bug, so I'm cc'ing 
bug-gnulib.

I installed the first three attached patches into Gnulib. The first one 
should fix the bug there, and the other two are refactoring cleanups.

I installed the last two attached patches into coreutils to propagate 
this fix into coreutils and add a regression test.

[-- Attachment #2: 0001-backupfile-fix-numbered-backups-for-XXX.patch --]
[-- Type: text/x-patch, Size: 5878 bytes --]

From d1b9e7c84959fb0f7014f03fc8746caa125f3d55 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 10 Dec 2021 13:31:02 -0800
Subject: [PATCH 1/3] backupfile: fix numbered backups for XXX/
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes Bug#52410 reported by Vincent Vermilya.
* lib/backupfile.c (check_extension): Return a bool indicating
whether the original extension was OK.  Caller changed.
(numbered_backup): Require that FILELEN does not count
trailing slashes after a non-slash, and don’t require
that *BUF be null-terminated.  Caller changed.
This fixes a bug where the numbered backup file name for X/ was
incorrectly computed because the trailing slash messed up the code
looking for X.~1~, X.~2~, etc., and this caused numbered_backup
to loop forever.  Also, check that check_extension doesn’t
truncate a file name leading to a different infloop.
---
 ChangeLog        | 14 ++++++++++++++
 lib/backupfile.c | 29 ++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bb3edbe44a..d012fb9488 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2021-12-10  Paul Eggert  <eggert@cs.ucla.edu>
+
+	backupfile: fix numbered backups for XXX/
+	* lib/backupfile.c (check_extension): Return a bool indicating
+	whether the original extension was OK.  Caller changed.
+	(numbered_backup): Require that FILELEN does not count
+	trailing slashes after a non-slash, and don’t require
+	that *BUF be null-terminated.  Caller changed.
+	This fixes a bug where the numbered backup file name for X/ was
+	incorrectly computed because the slash messed up the code
+	looking for X.~1~, X.~2~, etc., and this caused numbered_backup
+	to loop forever.  Also, check that check_extension doesn’t
+	truncate a file name leading to an infloop.
+
 2021-12-07  Paul Eggert  <eggert@cs.ucla.edu>
 
 	regex: pacify Coverity clean_state_log_if_needed
diff --git a/lib/backupfile.c b/lib/backupfile.c
index bc03dd6146..488eecfd82 100644
--- a/lib/backupfile.c
+++ b/lib/backupfile.c
@@ -91,13 +91,14 @@ set_simple_backup_suffix (char const *s)
 /* If FILE (which was of length FILELEN before an extension was
    appended to it) is too long, replace the extension with the single
    char E.  If the result is still too long, remove the char just
-   before E.
+   before E.  Return true if the extension was OK already, false
+   if it needed replacement.
 
    If DIR_FD is nonnegative, it is a file descriptor for FILE's parent.
-   *NAME_MAX is either 0, or the cached result of a previous call for
+   *BASE_MAX is either 0, or the cached result of a previous call for
    FILE's parent's _PC_NAME_MAX.  */
 
-static void
+static bool
 check_extension (char *file, size_t filelen, char e,
                  int dir_fd, size_t *base_max)
 {
@@ -153,13 +154,16 @@ check_extension (char *file, size_t filelen, char e,
         }
     }
 
-  if (baselen_max < baselen)
+  if (baselen <= baselen_max)
+    return true;
+  else
     {
       baselen = file + filelen - base;
       if (baselen_max <= baselen)
         baselen = baselen_max - 1;
       base[baselen] = e;
       base[baselen + 1] = '\0';
+      return false;
     }
 }
 
@@ -187,9 +191,11 @@ enum numbered_backup_result
    Store into *BUFFER the next backup name for the named file,
    with a version number greater than all the
    existing numbered backups.  Reallocate *BUFFER as necessary; its
-   initial allocated size is BUFFER_SIZE, which must be at least 4
+   initial allocated size is BUFFER_SIZE, which must be at least 5
    bytes longer than the file name to make room for the initially
-   appended ".~1".  FILELEN is the length of the original file name.
+   appended ".~1~".  FILELEN is the length of the original file name.
+   (The original file name is not necessarily null-terminated;
+   FILELEN does not count trailing slashes after a non-slash.)
    BASE_OFFSET is the offset of the basename in *BUFFER.
    The returned value indicates what kind of backup was found.  If an
    I/O or other read error occurs, use the highest backup number that
@@ -209,7 +215,7 @@ numbered_backup (int dir_fd, char **buffer, size_t buffer_size, size_t filelen,
   char *buf = *buffer;
   size_t versionlenmax = 1;
   char *base = buf + base_offset;
-  size_t baselen = base_len (base);
+  size_t baselen = filelen - base_offset;
 
   if (dirp)
     rewinddir (dirp);
@@ -312,7 +318,7 @@ backupfile_internal (int dir_fd, char const *file,
                      enum backup_type backup_type, bool rename)
 {
   idx_t base_offset = last_component (file) - file;
-  size_t filelen = base_offset + strlen (file + base_offset);
+  size_t filelen = base_offset + base_len (file + base_offset);
 
   if (! simple_backup_suffix)
     set_simple_backup_suffix (NULL);
@@ -335,7 +341,8 @@ backupfile_internal (int dir_fd, char const *file,
   size_t base_max = 0;
   while (true)
     {
-      memcpy (s, file, filelen + 1);
+      bool extended = true;
+      memcpy (s, file, filelen);
 
       if (backup_type == simple_backups)
         memcpy (s + filelen, simple_backup_suffix, simple_backup_suffix_size);
@@ -355,7 +362,7 @@ backupfile_internal (int dir_fd, char const *file,
               }
             FALLTHROUGH;
           case BACKUP_IS_LONGER:
-            check_extension (s, filelen, '~', sdir, &base_max);
+            extended = check_extension (s, filelen, '~', sdir, &base_max);
             break;
 
           case BACKUP_NOMEM:
@@ -378,7 +385,7 @@ backupfile_internal (int dir_fd, char const *file,
       if (renameatu (AT_FDCWD, file, sdir, s + base_offset, flags) == 0)
         break;
       int e = errno;
-      if (e != EEXIST)
+      if (! (e == EEXIST && extended))
         {
           if (dirp)
             closedir (dirp);
-- 
2.33.1


[-- Attachment #3: 0002-backupfile-prefer-signed-integers.patch --]
[-- Type: text/x-patch, Size: 5756 bytes --]

From c0dc5047980aaddd0d99096362147c24cb58aa75 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 10 Dec 2021 13:43:10 -0800
Subject: [PATCH 2/3] backupfile: prefer signed integers

* lib/backupfile.c: Include ialloc.h instead of idx.h.
Prefer idx_t to size_t where either will do.
Use imalloc and irealloc instead of malloc and realloc.
* modules/backupfile, modules/backup-rename (Depends-on):
Depend on ialloc not idx.
---
 ChangeLog             |  7 +++++++
 lib/backupfile.c      | 36 ++++++++++++++++++------------------
 modules/backup-rename |  2 +-
 modules/backupfile    |  2 +-
 4 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index d012fb9488..bb69d3246d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2021-12-10  Paul Eggert  <eggert@cs.ucla.edu>
 
+	backupfile: prefer signed integers
+	* lib/backupfile.c: Include ialloc.h instead of idx.h.
+	Prefer idx_t to size_t where either will do.
+	Use imalloc and irealloc instead of malloc and realloc.
+	* modules/backupfile, modules/backup-rename (Depends-on):
+	Depend on ialloc not idx.
+
 	backupfile: fix numbered backups for XXX/
 	* lib/backupfile.c (check_extension): Return a bool indicating
 	whether the original extension was OK.  Caller changed.
diff --git a/lib/backupfile.c b/lib/backupfile.c
index 488eecfd82..6667fd822f 100644
--- a/lib/backupfile.c
+++ b/lib/backupfile.c
@@ -33,7 +33,7 @@
 
 #include "attribute.h"
 #include "basename-lgpl.h"
-#include "idx.h"
+#include "ialloc.h"
 #include "intprops.h"
 #include "opendirat.h"
 #include "renameatu.h"
@@ -99,12 +99,12 @@ set_simple_backup_suffix (char const *s)
    FILE's parent's _PC_NAME_MAX.  */
 
 static bool
-check_extension (char *file, size_t filelen, char e,
-                 int dir_fd, size_t *base_max)
+check_extension (char *file, idx_t filelen, char e,
+                 int dir_fd, idx_t *base_max)
 {
   char *base = last_component (file);
-  size_t baselen = base_len (base);
-  size_t baselen_max = HAVE_LONG_FILE_NAMES ? 255 : NAME_MAX_MINIMUM;
+  idx_t baselen = base_len (base);
+  idx_t baselen_max = HAVE_LONG_FILE_NAMES ? 255 : NAME_MAX_MINIMUM;
 
   if (HAVE_DOS_FILE_NAMES || NAME_MAX_MINIMUM < baselen)
     {
@@ -206,16 +206,16 @@ enum numbered_backup_result
    and its file descriptor into *PNEW_FD without closing the stream.  */
 
 static enum numbered_backup_result
-numbered_backup (int dir_fd, char **buffer, size_t buffer_size, size_t filelen,
+numbered_backup (int dir_fd, char **buffer, idx_t buffer_size, idx_t filelen,
                  idx_t base_offset, DIR **dirpp, int *pnew_fd)
 {
   enum numbered_backup_result result = BACKUP_IS_NEW;
   DIR *dirp = *dirpp;
   struct dirent *dp;
   char *buf = *buffer;
-  size_t versionlenmax = 1;
+  idx_t versionlenmax = 1;
   char *base = buf + base_offset;
-  size_t baselen = filelen - base_offset;
+  idx_t baselen = filelen - base_offset;
 
   if (dirp)
     rewinddir (dirp);
@@ -241,7 +241,7 @@ numbered_backup (int dir_fd, char **buffer, size_t buffer_size, size_t filelen,
       char const *p;
       char *q;
       bool all_9s;
-      size_t versionlen;
+      idx_t versionlen;
 
       if (_D_EXACT_NAMLEN (dp) < baselen + 4)
         continue;
@@ -273,13 +273,13 @@ numbered_backup (int dir_fd, char **buffer, size_t buffer_size, size_t filelen,
 
       versionlenmax = all_9s + versionlen;
       result = (all_9s ? BACKUP_IS_LONGER : BACKUP_IS_SAME_LENGTH);
-      size_t new_buffer_size = filelen + 2 + versionlenmax + 2;
+      idx_t new_buffer_size = filelen + 2 + versionlenmax + 2;
       if (buffer_size < new_buffer_size)
         {
-          size_t grown;
+          idx_t grown;
           if (! INT_ADD_WRAPV (new_buffer_size, new_buffer_size >> 1, &grown))
             new_buffer_size = grown;
-          char *new_buf = realloc (buf, new_buffer_size);
+          char *new_buf = irealloc (buf, new_buffer_size);
           if (!new_buf)
             {
               *buffer = buf;
@@ -318,27 +318,27 @@ backupfile_internal (int dir_fd, char const *file,
                      enum backup_type backup_type, bool rename)
 {
   idx_t base_offset = last_component (file) - file;
-  size_t filelen = base_offset + base_len (file + base_offset);
+  idx_t filelen = base_offset + base_len (file + base_offset);
 
   if (! simple_backup_suffix)
     set_simple_backup_suffix (NULL);
 
   /* Allow room for simple or ".~N~" backups.  The guess must be at
      least sizeof ".~1~", but otherwise will be adjusted as needed.  */
-  size_t simple_backup_suffix_size = strlen (simple_backup_suffix) + 1;
-  size_t backup_suffix_size_guess = simple_backup_suffix_size;
+  idx_t simple_backup_suffix_size = strlen (simple_backup_suffix) + 1;
+  idx_t backup_suffix_size_guess = simple_backup_suffix_size;
   enum { GUESS = sizeof ".~12345~" };
   if (backup_suffix_size_guess < GUESS)
     backup_suffix_size_guess = GUESS;
 
-  ssize_t ssize = filelen + backup_suffix_size_guess + 1;
-  char *s = malloc (ssize);
+  idx_t ssize = filelen + backup_suffix_size_guess + 1;
+  char *s = imalloc (ssize);
   if (!s)
     return s;
 
   DIR *dirp = NULL;
   int sdir = -1;
-  size_t base_max = 0;
+  idx_t base_max = 0;
   while (true)
     {
       bool extended = true;
diff --git a/modules/backup-rename b/modules/backup-rename
index c50e874ff8..245350a4df 100644
--- a/modules/backup-rename
+++ b/modules/backup-rename
@@ -16,7 +16,7 @@ c99
 closedir
 d-ino
 fcntl-h
-idx
+ialloc
 intprops
 memcmp
 opendirat
diff --git a/modules/backupfile b/modules/backupfile
index 342a7bdff1..3d06da9ed4 100644
--- a/modules/backupfile
+++ b/modules/backupfile
@@ -16,7 +16,7 @@ c99
 closedir
 d-ino
 fcntl-h
-idx
+ialloc
 intprops
 memcmp
 opendirat
-- 
2.33.1


[-- Attachment #4: 0003-backupfile-assume-C99-decls.patch --]
[-- Type: text/x-patch, Size: 3194 bytes --]

From 94bc13f08d9cff319f4611597a73c6d632df3511 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 10 Dec 2021 13:54:54 -0800
Subject: [PATCH 3/3] backupfile: assume C99 decls

* lib/backupfile.c: Use C99-style decls after statements.
---
 ChangeLog        |  4 ++++
 lib/backupfile.c | 17 ++++++-----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index bb69d3246d..650fc090ed 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,9 +1,13 @@
 2021-12-10  Paul Eggert  <eggert@cs.ucla.edu>
 
+	backupfile: assume C99 decls
+	* lib/backupfile.c: Use C99-style decls after statements.
+
 	backupfile: prefer signed integers
 	* lib/backupfile.c: Include ialloc.h instead of idx.h.
 	Prefer idx_t to size_t where either will do.
 	Use imalloc and irealloc instead of malloc and realloc.
+
 	* modules/backupfile, modules/backup-rename (Depends-on):
 	Depend on ialloc not idx.
 
diff --git a/lib/backupfile.c b/lib/backupfile.c
index 6667fd822f..ce74488534 100644
--- a/lib/backupfile.c
+++ b/lib/backupfile.c
@@ -211,10 +211,8 @@ numbered_backup (int dir_fd, char **buffer, idx_t buffer_size, idx_t filelen,
 {
   enum numbered_backup_result result = BACKUP_IS_NEW;
   DIR *dirp = *dirpp;
-  struct dirent *dp;
   char *buf = *buffer;
   idx_t versionlenmax = 1;
-  char *base = buf + base_offset;
   idx_t baselen = filelen - base_offset;
 
   if (dirp)
@@ -224,6 +222,7 @@ numbered_backup (int dir_fd, char **buffer, idx_t buffer_size, idx_t filelen,
       /* Temporarily modify the buffer into its parent directory name,
          open the directory, and then restore the buffer.  */
       char tmp[sizeof "."];
+      char *base = buf + base_offset;
       memcpy (tmp, base, sizeof ".");
       strcpy (base, ".");
       dirp = opendirat (dir_fd, buf, 0, pnew_fd);
@@ -236,20 +235,15 @@ numbered_backup (int dir_fd, char **buffer, idx_t buffer_size, idx_t filelen,
       *dirpp = dirp;
     }
 
-  while ((dp = readdir (dirp)) != NULL)
+  for (struct dirent *dp; (dp = readdir (dirp)) != NULL; )
     {
-      char const *p;
-      char *q;
-      bool all_9s;
-      idx_t versionlen;
-
       if (_D_EXACT_NAMLEN (dp) < baselen + 4)
         continue;
 
       if (memcmp (buf + base_offset, dp->d_name, baselen + 2) != 0)
         continue;
 
-      p = dp->d_name + baselen + 2;
+      char const *p = dp->d_name + baselen + 2;
 
       /* Check whether this file has a version number and if so,
          whether it is larger.  Use string operations rather than
@@ -257,7 +251,8 @@ numbered_backup (int dir_fd, char **buffer, idx_t buffer_size, idx_t filelen,
 
       if (! ('1' <= *p && *p <= '9'))
         continue;
-      all_9s = (*p == '9');
+      bool all_9s = (*p == '9');
+      idx_t versionlen;
       for (versionlen = 1; ISDIGIT (p[versionlen]); versionlen++)
         all_9s &= (p[versionlen] == '9');
 
@@ -288,7 +283,7 @@ numbered_backup (int dir_fd, char **buffer, idx_t buffer_size, idx_t filelen,
           buf = new_buf;
           buffer_size = new_buffer_size;
         }
-      q = buf + filelen;
+      char *q = buf + filelen;
       *q++ = '.';
       *q++ = '~';
       *q = '0';
-- 
2.33.1


[-- Attachment #5: 0001-build-update-gnulib-submodule-to-latest.patch --]
[-- Type: text/x-patch, Size: 511 bytes --]

From 71444f3ea769a3fc5d6a57e00ae7556df3480052 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 10 Dec 2021 13:59:01 -0800
Subject: [PATCH 1/2] build: update gnulib submodule to latest

---
 gnulib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnulib b/gnulib
index 1a268176f..4d1e184fd 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 1a268176fbb184e393c98575e61fe692264c7d91
+Subproject commit 4d1e184fd546db46ca11bb4974e47984366875d9
-- 
2.33.1


[-- Attachment #6: 0002-mv-Bug-52410-fix.patch --]
[-- Type: text/x-patch, Size: 1283 bytes --]

From 9206dbeb5d3e87cfca373c5c1e9c32a14e5ce592 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 10 Dec 2021 14:08:58 -0800
Subject: [PATCH 2/2] mv: Bug#52410 fix

The recent Gnulib update fixed this bug reported by Vincent Vermilya.
* tests/mv/backup-dir.sh: Test for Bug#52410.
---
 NEWS                   | 4 ++++
 tests/mv/backup-dir.sh | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/NEWS b/NEWS
index 61f58b668..c2ecdedfa 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   and B is in some other file system.
   [bug introduced in coreutils-9.0]
 
+  'mv -T --backup=numbered A B/' no longer miscalculates the backup number
+  for B when A is a directory, possibly inflooping.
+  [bug introduced in coreutils-6.3]
+
 ** Changes in behavior
 
   timeout --foreground --kill-after=... will now exit with status 137
diff --git a/tests/mv/backup-dir.sh b/tests/mv/backup-dir.sh
index f920f319d..045228f4e 100755
--- a/tests/mv/backup-dir.sh
+++ b/tests/mv/backup-dir.sh
@@ -31,4 +31,9 @@ EOF
 
 compare exp out || fail=1
 
+# Bug#52410
+mkdir C D E || framework_failure_
+mv -T --backup=numbered C E/ || fail=1
+mv -T --backup=numbered D E/ || fail=1
+
 Exit $fail
-- 
2.33.1