[svn] Fix a possible race condition when opening files.

Published in <87r7j6vy9g.fsf@xemacs.org>.

diff --git a/src/ChangeLog b/src/ChangeLog
index 90a79d4dcd26a6c4960f46c62d2783df93c6de96..277a0edb8b85f0d5e8294ae9ab996b45acb7702b 100644 (file)
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,23 @@
+2005-02-24  Hrvoje Niksic  <hniksic@xemacs.org>
+
+       * ftp.c (getftp): Ditto.
+
+       * http.c (gethttp): When we're not supposed to overwrite files,
+       use fopen_excl to open the file and recompute the file name.
+
+       * log.c (redirect_output): Use unique_create to avoid a race
+       condition.
+
+       * mswindows.c (fake_fork_child): Use unique_create.
+
+       * utils.c (fopen_excl): New function that opens a stdio stream
+       with the O_EXCL flag (where available).
+       (unique_create): New function, like unique_name, but also creating
+       the file and returning a file pointer.
+       (fork_to_background): Use unique_create to create the file
+       immediately to avoid race condition with multiple instances of
+       wget -b.
+

 2005-02-24  Hrvoje Niksic  <hniksic@xemacs.org>
 
        * host.c (lookup_host): Test for AI_ADDRCONFIG directly, instead
 2005-02-24  Hrvoje Niksic  <hniksic@xemacs.org>
 
        * host.c (lookup_host): Test for AI_ADDRCONFIG directly, instead

diff --git a/src/ftp.c b/src/ftp.c
index e65f8284dccae3a3d835f41410b37817cb95e2dd..c1e2d002c7cd7244e9032aab07385b6f29a4cb4f 100644 (file)
--- a/src/ftp.c
+++ b/src/ftp.c
@@ -953,10 +953,30 @@ Error in server response, closing control connection.\n"));
       mkalldirs (con->target);
       if (opt.backups)
        rotate_backups (con->target);
       mkalldirs (con->target);
       if (opt.backups)
        rotate_backups (con->target);

-      /* #### Is this correct? */
-      chmod (con->target, 0600);

 
 

-      fp = fopen (con->target, restval ? "ab" : "wb");
+      if (restval)
+       fp = fopen (con->target, "ab");
+      else if (opt.noclobber || opt.always_rest || opt.timestamping || opt.dirstruct
+              || opt.output_document)
+       fp = fopen (con->target, "wb");
+      else
+       {
+         fp = fopen_excl (con->target, 0);
+         if (!fp && errno == EEXIST)
+           {
+             /* We cannot just invent a new name and use it (which is
+                what functions like unique_create typically do)
+                because we told the user we'd use this name.
+                Instead, return and retry the download.  */
+             logprintf (LOG_NOTQUIET, _("%s has sprung into existence.\n"),
+                        con->target);
+             fd_close (csock);
+             con->csock = -1;
+             fd_close (dtsock);
+             fd_close (local_sock);
+             return FOPEN_EXCL_ERR;
+           }
+       }

       if (!fp)
        {
          logprintf (LOG_NOTQUIET, "%s: %s\n", con->target, strerror (errno));
       if (!fp)
        {
          logprintf (LOG_NOTQUIET, "%s: %s\n", con->target, strerror (errno));


@@ -1219,8 +1239,16 @@ ftp_loop_internal (struct url *u, struct fileinfo *f, ccon *con)
        case CONSOCKERR: case CONERROR: case FTPSRVERR: case FTPRERR:
        case WRITEFAILED: case FTPUNKNOWNTYPE: case FTPSYSERR:
        case FTPPORTERR: case FTPLOGREFUSED: case FTPINVPASV:
        case CONSOCKERR: case CONERROR: case FTPSRVERR: case FTPRERR:
        case WRITEFAILED: case FTPUNKNOWNTYPE: case FTPSYSERR:
        case FTPPORTERR: case FTPLOGREFUSED: case FTPINVPASV:

+       case FOPEN_EXCL_ERR:

          printwhat (count, opt.ntry);
          /* non-fatal errors */
          printwhat (count, opt.ntry);
          /* non-fatal errors */

+         if (err == FOPEN_EXCL_ERR)
+           {
+             /* Re-determine the file name. */
+             xfree_null (con->target);
+             con->target = url_file_name (u);
+             locf = con->target;
+           }

          continue;
          break;
        case FTPRETRINT:
          continue;
          break;
        case FTPRETRINT:

diff --git a/src/http.c b/src/http.c
index 93f072b53e3aaaf899104562d52551db4fbd4392..6bc4f3be5be0a41994c7bbf57bf837b2ed8d9540 100644 (file)
--- a/src/http.c
+++ b/src/http.c
@@ -1720,7 +1720,27 @@ gethttp (struct url *u, struct http_stat *hs, int *dt, struct url *proxy)
       mkalldirs (*hs->local_file);
       if (opt.backups)
        rotate_backups (*hs->local_file);
       mkalldirs (*hs->local_file);
       if (opt.backups)
        rotate_backups (*hs->local_file);

-      fp = fopen (*hs->local_file, hs->restval ? "ab" : "wb");
+      if (hs->restval)
+       fp = fopen (*hs->local_file, "ab");
+      else if (opt.noclobber || opt.always_rest || opt.timestamping || opt.dirstruct
+              || opt.output_document)
+       fp = fopen (*hs->local_file, "wb");
+      else
+       {
+         fp = fopen_excl (*hs->local_file, 0);
+         if (!fp && errno == EEXIST)
+           {
+             /* We cannot just invent a new name and use it (which is
+                what functions like unique_create typically do)
+                because we told the user we'd use this name.
+                Instead, return and retry the download.  */
+             logprintf (LOG_NOTQUIET,
+                        _("%s has sprung into existence.\n"),
+                        *hs->local_file);
+             CLOSE_INVALIDATE (sock);
+             return FOPEN_EXCL_ERR;
+           }
+       }

       if (!fp)
        {
          logprintf (LOG_NOTQUIET, "%s: %s\n", *hs->local_file, strerror (errno));
       if (!fp)
        {
          logprintf (LOG_NOTQUIET, "%s: %s\n", *hs->local_file, strerror (errno));


@@ -1992,12 +2012,35 @@ File `%s' already there, will not retrieve.\n"), *hstat.local_file);
        {
        case HERR: case HEOF: case CONSOCKERR: case CONCLOSED:
        case CONERROR: case READERR: case WRITEFAILED:
        {
        case HERR: case HEOF: case CONSOCKERR: case CONCLOSED:
        case CONERROR: case READERR: case WRITEFAILED:

-       case RANGEERR:
+       case RANGEERR: case FOPEN_EXCL_ERR:

          /* Non-fatal errors continue executing the loop, which will
             bring them to "while" statement at the end, to judge
             whether the number of tries was exceeded.  */
          free_hstat (&hstat);
          printwhat (count, opt.ntry);
          /* Non-fatal errors continue executing the loop, which will
             bring them to "while" statement at the end, to judge
             whether the number of tries was exceeded.  */
          free_hstat (&hstat);
          printwhat (count, opt.ntry);

+         if (err == FOPEN_EXCL_ERR)
+           {
+             /* Re-determine the file name. */
+             if (local_file && *local_file)
+               {
+                 xfree (*local_file);
+                 *local_file = url_file_name (u);
+                 hstat.local_file = local_file;
+               }
+             else
+               {
+                 xfree (dummy);
+                 dummy = url_file_name (u);
+                 hstat.local_file = &dummy;
+               }
+             /* be honest about where we will save the file */
+             if (local_file && opt.output_document)
+               *local_file = HYPHENP (opt.output_document) ? NULL : xstrdup (opt.output_document);
+             if (!opt.output_document)
+               locf = *hstat.local_file;
+             else
+               locf = opt.output_document;
+           }

          continue;
          break;
        case HOSTERR: case CONIMPOSSIBLE: case PROXERR: case AUTHFAILED: 
          continue;
          break;
        case HOSTERR: case CONIMPOSSIBLE: case PROXERR: case AUTHFAILED: 

diff --git a/src/log.c b/src/log.c
index 96582d522a640008850df1d097270b055f51bdfb..2511513fc0ef95bb9ab0763d9a22499188508554 100644 (file)
--- a/src/log.c
+++ b/src/log.c
@@ -625,24 +625,25 @@ static const char *redirect_request_signal_name;
 static void
 redirect_output (void)
 {
 static void
 redirect_output (void)
 {

-  char *logfile = unique_name (DEFAULT_LOGFILE, 0);
-  fprintf (stderr, _("\n%s received, redirecting output to `%s'.\n"),
-          redirect_request_signal_name, logfile);
-  logfp = fopen (logfile, "w");
-  if (!logfp)
+  char *logfile;
+  logfp = unique_create (DEFAULT_LOGFILE, 0, &logfile);
+  if (logfp)
+    {
+      fprintf (stderr, _("\n%s received, redirecting output to `%s'.\n"),
+              redirect_request_signal_name, logfile);
+      xfree (logfile);
+      /* Dump the context output to the newly opened log.  */
+      log_dump_context ();
+    }
+  else

     {
       /* Eek!  Opening the alternate log file has failed.  Nothing we
          can do but disable printing completely. */
     {
       /* Eek!  Opening the alternate log file has failed.  Nothing we
          can do but disable printing completely. */

+      fprintf (stderr, _("\n%s received.\n"), redirect_request_signal_name);

       fprintf (stderr, _("%s: %s; disabling logging.\n"),
               logfile, strerror (errno));
       inhibit_logging = 1;
     }
       fprintf (stderr, _("%s: %s; disabling logging.\n"),
               logfile, strerror (errno));
       inhibit_logging = 1;
     }

-  else
-    {
-      /* Dump the context output to the newly opened log.  */
-      log_dump_context ();
-    }
-  xfree (logfile);

   save_context_p = 0;
 }
 
   save_context_p = 0;
 }
 

diff --git a/src/mswindows.c b/src/mswindows.c
index ca1f46a8e541abece471743897b4de5855419e04..22605a95d8fa6b345824c28e6681a43c090117d5 100644 (file)
--- a/src/mswindows.c
+++ b/src/mswindows.c
@@ -230,18 +230,7 @@ ws_cleanup (void)
 static void
 ws_hangup (const char *reason)
 {
 static void
 ws_hangup (const char *reason)
 {

-  /* Whether we arrange our own version of opt.lfilename here.  */
-  int changedp = 0;
-
-  if (!opt.lfilename)
-    {
-      opt.lfilename = unique_name (DEFAULT_LOGFILE, 0);
-      changedp = 1;
-    }
-  printf (_("Continuing in background.\n"));
-  if (changedp)
-    printf (_("Output will be written to `%s'.\n"), opt.lfilename);
-
+  fprintf (stderr, _("Continuing in background.\n"));

   log_request_redirect_output (reason);
 
   /* Detach process from the current console.  Under Windows 9x, if we
   log_request_redirect_output (reason);
 
   /* Detach process from the current console.  Under Windows 9x, if we


@@ -265,7 +254,7 @@ make_section_name (DWORD pid)
 struct fake_fork_info
 {
   HANDLE event;
 struct fake_fork_info
 {
   HANDLE event;

-  int changedp;
+  int logfile_changed;

   char lfilename[MAX_PATH + 1];
 };
 
   char lfilename[MAX_PATH + 1];
 };
 


@@ -300,15 +289,19 @@ fake_fork_child (void)
 
   event = info->event;
 
 
   event = info->event;
 

+  info->logfile_changed = 0;

   if (!opt.lfilename)
     {
   if (!opt.lfilename)
     {

-      opt.lfilename = unique_name (DEFAULT_LOGFILE, 0);
-      info->changedp = 1;
-      strncpy (info->lfilename, opt.lfilename, sizeof (info->lfilename));
-      info->lfilename[sizeof (info->lfilename) - 1] = '\0';
+      /* See utils:fork_to_background for explanation. */
+      FILE *new_log_fp = unique_create (DEFAULT_LOGFILE, 0, &opt.lfilename);
+      if (new_log_fp)
+       {
+         info->logfile_changed = 1;
+         strncpy (info->lfilename, opt.lfilename, sizeof (info->lfilename));
+         info->lfilename[sizeof (info->lfilename) - 1] = '\0';
+         fclose (new_log_fp);
+       }

     }
     }

-  else
-    info->changedp = 0;

 
   UnmapViewOfFile (info);
   CloseHandle (section);
 
   UnmapViewOfFile (info);
   CloseHandle (section);


@@ -422,7 +415,7 @@ fake_fork (void)
     }
 
   /* Ensure string is properly terminated.  */
     }
 
   /* Ensure string is properly terminated.  */

-  if (info->changedp &&
+  if (info->logfile_changed &&

       !memchr (info->lfilename, '\0', sizeof (info->lfilename)))
     {
       rv = FALSE;
       !memchr (info->lfilename, '\0', sizeof (info->lfilename)))
     {
       rv = FALSE;


@@ -430,7 +423,7 @@ fake_fork (void)
     }
 
   printf (_("Continuing in background, pid %lu.\n"), pi.dwProcessId);
     }
 
   printf (_("Continuing in background, pid %lu.\n"), pi.dwProcessId);

-  if (info->changedp)
+  if (info->logfile_changed)

     printf (_("Output will be written to `%s'.\n"), info->lfilename);
 
   UnmapViewOfFile (info);
     printf (_("Output will be written to `%s'.\n"), info->lfilename);
 
   UnmapViewOfFile (info);

diff --git a/src/utils.c b/src/utils.c
index 2589a233234098f390492585dc3b999930133fe4..0fe30bfd38e494b6ea0991733ba948a4a7495bb4 100644 (file)
--- a/src/utils.c
+++ b/src/utils.c
@@ -283,12 +283,21 @@ fork_to_background (void)
 {
   pid_t pid;
   /* Whether we arrange our own version of opt.lfilename here.  */
 {
   pid_t pid;
   /* Whether we arrange our own version of opt.lfilename here.  */

-  int changedp = 0;
+  int logfile_changed = 0;

 
   if (!opt.lfilename)
     {
 
   if (!opt.lfilename)
     {

-      opt.lfilename = unique_name (DEFAULT_LOGFILE, 0);
-      changedp = 1;
+      /* We must create the file immediately to avoid either a race
+        condition (which arises from using unique_name and failing to
+        use fopen_excl) or lying to the user about the log file name
+        (which arises from using unique_name, printing the name, and
+        using fopen_excl later on.)  */
+      FILE *new_log_fp = unique_create (DEFAULT_LOGFILE, 0, &opt.lfilename);
+      if (new_log_fp)
+       {
+         logfile_changed = 1;
+         fclose (new_log_fp);
+       }

     }
   pid = fork ();
   if (pid < 0)
     }
   pid = fork ();
   if (pid < 0)


@@ -301,7 +310,7 @@ fork_to_background (void)
     {
       /* parent, no error */
       printf (_("Continuing in background, pid %d.\n"), (int)pid);
     {
       /* parent, no error */
       printf (_("Continuing in background, pid %d.\n"), (int)pid);

-      if (changedp)
+      if (logfile_changed)

        printf (_("Output will be written to `%s'.\n"), opt.lfilename);
       exit (0);                        /* #### should we use _exit()? */
     }
        printf (_("Output will be written to `%s'.\n"), opt.lfilename);
       exit (0);                        /* #### should we use _exit()? */
     }


@@ -439,7 +448,7 @@ unique_name_1 (const char *prefix)
    exist at the point in time when the function was called.
    Therefore, where security matters, don't rely that the file created
    by this function exists until you open it with O_EXCL or
    exist at the point in time when the function was called.
    Therefore, where security matters, don't rely that the file created
    by this function exists until you open it with O_EXCL or

-   something.
+   equivalent.

 
    If ALLOW_PASSTHROUGH is 0, it always returns a freshly allocated
    string.  Otherwise, it may return FILE if the file doesn't exist
 
    If ALLOW_PASSTHROUGH is 0, it always returns a freshly allocated
    string.  Otherwise, it may return FILE if the file doesn't exist


@@ -457,6 +466,65 @@ unique_name (const char *file, int allow_passthrough)
      and return it.  */
   return unique_name_1 (file);
 }
      and return it.  */
   return unique_name_1 (file);
 }

+
+/* Create a file based on NAME, except without overwriting an existing
+   file with that name.  Providing O_EXCL is correctly implemented,
+   this function does not have the race condition associated with
+   opening the file returned by unique_name.  */
+
+FILE *
+unique_create (const char *name, int binary, char **opened_name)
+{
+  /* unique file name, based on NAME */
+  char *uname = unique_name (name, 0);
+  FILE *fp;
+  while ((fp = fopen_excl (uname, binary)) == NULL && errno == EEXIST)
+    {
+      xfree (uname);
+      uname = unique_name (name, 0);
+    }
+  if (opened_name && fp != NULL)
+    {
+      if (fp)
+       *opened_name = uname;
+      else
+       {
+         *opened_name = NULL;
+         xfree (uname);
+       }
+    }
+  else
+    xfree (uname);
+  return fp;
+}
+
+/* Open the file for writing, with the addition that the file is
+   opened "exclusively".  This means that, if the file already exists,
+   this function will *fail* and errno will be set to EEXIST.  If
+   BINARY is set, the file will be opened in binary mode, equivalent
+   to fopen's "wb".
+
+   If opening the file fails for any reason, including the file having
+   previously existed, this function returns NULL and sets errno
+   appropriately.  */
+   
+FILE *
+fopen_excl (const char *fname, int binary)
+{
+#ifdef O_EXCL
+  int flags = O_WRONLY | O_CREAT | O_EXCL;
+# ifdef O_BINARY
+  if (binary)
+    flags |= O_BINARY
+# endif
+  int fd = open (fname, flags, 0666);
+  if (fd < 0)
+    return NULL;
+  return fdopen (fd, binary ? "wb" : "w");
+#else  /* not O_EXCL */
+  return fopen (fname, binary ? "wb" : "w");
+#endif /* not O_EXCL */
+}

 \f
 /* Create DIRECTORY.  If some of the pathname components of DIRECTORY
    are missing, create them first.  In case any mkdir() call fails,
 \f
 /* Create DIRECTORY.  If some of the pathname components of DIRECTORY
    are missing, create them first.  In case any mkdir() call fails,

diff --git a/src/utils.h b/src/utils.h
index 08aa5667d1a3bfe3a23d002493889f65580e9178..d22463edec3eb7f13d1aedf50dde8cf0001868d1 100644 (file)
--- a/src/utils.h
+++ b/src/utils.h
@@ -82,6 +82,8 @@ int file_non_directory_p PARAMS ((const char *));
 wgint file_size PARAMS ((const char *));
 int make_directory PARAMS ((const char *));
 char *unique_name PARAMS ((const char *, int));
 wgint file_size PARAMS ((const char *));
 int make_directory PARAMS ((const char *));
 char *unique_name PARAMS ((const char *, int));

+FILE *unique_create PARAMS ((const char *, int, char **));
+FILE *fopen_excl PARAMS ((const char *, int));

 char *file_merge PARAMS ((const char *, const char *));
 
 int acceptable PARAMS ((const char *));
 char *file_merge PARAMS ((const char *, const char *));
 
 int acceptable PARAMS ((const char *));

diff --git a/src/wget.h b/src/wget.h
index c41a6814d30e010d106d2ea5d705027dc20d3b3b..1ac6eccb0ebdbdd018ec82756d592f0a28e3e533 100644 (file)
--- a/src/wget.h
+++ b/src/wget.h
@@ -237,7 +237,7 @@ typedef enum
   CONCLOSED, FTPOK, FTPLOGINC, FTPLOGREFUSED, FTPPORTERR, FTPSYSERR,
   FTPNSFOD, FTPRETROK, FTPUNKNOWNTYPE, FTPRERR,
   FTPREXC, FTPSRVERR, FTPRETRINT, FTPRESTFAIL, URLERROR,
   CONCLOSED, FTPOK, FTPLOGINC, FTPLOGREFUSED, FTPPORTERR, FTPSYSERR,
   FTPNSFOD, FTPRETROK, FTPUNKNOWNTYPE, FTPRERR,
   FTPREXC, FTPSRVERR, FTPRETRINT, FTPRESTFAIL, URLERROR,

-  FOPENERR, FWRITEERR, HOK, HLEXC, HEOF,
+  FOPENERR, FOPEN_EXCL_ERR, FWRITEERR, HOK, HLEXC, HEOF,

   HERR, RETROK, RECLEVELEXC, FTPACCDENIED, WRONGCODE,
   FTPINVPASV, FTPNOPASV,
   CONTNOTSUPPORTED, RETRUNNEEDED, RETRFINISHED, READERR, TRYLIMEXC,
   HERR, RETROK, RECLEVELEXC, FTPACCDENIED, WRONGCODE,
   FTPINVPASV, FTPNOPASV,
   CONTNOTSUPPORTED, RETRUNNEEDED, RETRFINISHED, READERR, TRYLIMEXC,