GNU bug report logs - #25342
ln: avoid race condition with "ln -f src dst"

Previous Next

Full log

View this message in rfc822 format

From: Mirsad Goran Todorovac <mtodorov3_69 <at> yahoo.com>
To: 25342 <at> debbugs.gnu.org
Subject: bug#25342: GNU coreutils: race condition in "ln -f source dest"
Date: Mon, 2 Jan 2017 21:58:35 +0000 (UTC)

[Message part 1 (text/plain, inline)]

When "dest" is existing, ln -f source dest does an attempt of linkat(AT_FCWDFD, source, AT_FCWDFD, dest), which fails with EEXIST if dest exists, and then attempts an unlink(dest), followed by repeated linkat().
Between unlink() and second linkat() process maybe preempted, leaving no system file.

An option -o (--force-overwrite) is proposed, so existing scripts would work w/o breaking, and linkat() can be made atomic. By rights, there oughta be an option for linkat() to do it, but right now it was done by spurious idea (Thanking the grace of Lord Merciful and Longsuffering).
Patch follows.
All the best,Mirsad X. Todorovac

__________________________________________________________________________________________________
marvin <at> marvin-desktop:~$ diff -r -U 3 coreutils-8.26 coreutils-8.26p1
diff -r -U 3 coreutils-8.26/src/ln.c coreutils-8.26p1/src/ln.c
--- coreutils-8.26/src/ln.c    2016-11-14 13:05:20.000000000 +0100
+++ coreutils-8.26p1/src/ln.c    2017-01-02 22:23:53.608194338 +0100
@@ -59,6 +59,9 @@
 /* If true, remove existing files unconditionally.  */
 static bool remove_existing_files;
 
+/* If true, remove existing files unconditionally, even if losing data.  */
+static bool force_remove_existing_files;
+
 /* If true, list each file as it is moved. */
 static bool verbose;
 
@@ -90,6 +93,7 @@
   {"no-dereference", no_argument, NULL, 'n'},
   {"no-target-directory", no_argument, NULL, 'T'},
   {"force", no_argument, NULL, 'f'},
+  {"force-overwrite", no_argument, NULL, 'o'},
   {"interactive", no_argument, NULL, 'i'},
   {"suffix", required_argument, NULL, 'S'},
   {"target-directory", required_argument, NULL, 't'},
@@ -283,7 +287,8 @@
       if (backup_type != no_backups)
         {
           dest_backup = find_backup_file_name (dest, backup_type);
-          if (rename (dest, dest_backup) != 0)
+      if (logical && rename (dest, dest_backup) != 0 ||
+              !logical && link (dest, dest_backup) != 0)
             {
               int rename_errno = errno;
               free (dest_backup);
@@ -301,10 +306,31 @@
   if (relative)
     source = rel_source = convert_abs_rel (source, dest);
 
-  ok = ((symbolic_link ? symlink (source, dest)
+  if (logical || !force_remove_existing_files)
+    ok = ((symbolic_link ? symlink (source, dest)
          : linkat (AT_FDCWD, source, AT_FDCWD, dest,
                    logical ? AT_SYMLINK_FOLLOW : 0))
         == 0);
+  else {
+    char *tmpname = NULL;
+
+    if (force_remove_existing_files &&
+        asprintf (&tmpname, "%s.%06d.bak", source, (int)getpid()) < strlen(source) + 1 + 6 + 4) {
+      int asprintf_errno = errno;
+      if (tmpname)
+    free (tmpname);
+      free (dest_backup);
+      free (rel_source);
+      error (0, asprintf_errno, _("cannot backup %s: cannot allocate temp string"),
+             quoteaf (dest));
+      return false;
+    }
+    
+    ok = ((symbolic_link ? symlink (source, dest)
+         : linkat (AT_FDCWD, source, AT_FDCWD, tmpname, 0) == 0
+           && renameat (AT_FDCWD, tmpname, AT_FDCWD, dest) == 0));
+  }
+
 
   /* If the attempt to create a link failed and we are removing or
      backing up destinations, unlink the destination and try again.
@@ -417,6 +443,7 @@
                                 directories (note: will probably fail due to\n\
                                 system restrictions, even for the superuser)\n\
   -f, --force                 remove existing destination files\n\
+  -o, --overwrite             force remove existing destination files\n\
 "), stdout);
       fputs (_("\
   -i, --interactive           prompt whether to remove destinations\n\
@@ -467,10 +494,10 @@
 
   atexit (close_stdin);
 
-  symbolic_link = remove_existing_files = interactive = verbose
+  symbolic_link = force_remove_existing_files = remove_existing_files = interactive = verbose
     = hard_dir_link = false;
 
-  while ((c = getopt_long (argc, argv, "bdfinrst:vFLPS:T", long_options, NULL))
+  while ((c = getopt_long (argc, argv, "bdfinorst:vFLPS:T", long_options, NULL))
          != -1)
     {
       switch (c)
@@ -484,6 +511,8 @@
         case 'F':
           hard_dir_link = true;
           break;
+        case 'o':
+          force_remove_existing_files = true;
         case 'f':
           remove_existing_files = true;
           interactive = false;
marvin <at> marvin-desktop:~$ 

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.