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

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

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

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);
-      /* #### 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));
@@ -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 FOPEN_EXCL_ERR:
 	  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:

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);
-      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));
@@ -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 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);
+	  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: 

src/log.c

@@ -625,24 +625,25 @@ static const char *redirect_request_signal_name;
 static void
 redirect_output (void)
 {
-  char *logfile = unique_name (DEFAULT_LOGFILE, 0);
+  char *logfile;
-  fprintf (stderr, _("\n%s received, redirecting output to `%s'.\n"),
+  logfp = unique_create (DEFAULT_LOGFILE, 0, &logfile);
-	   redirect_request_signal_name, logfile);
+  if (logfp)
-  logfp = fopen (logfile, "w");
+    {
-  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. */
+      fprintf (stderr, _("\n%s received.\n"), redirect_request_signal_name);
       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;
 }
 

src/mswindows.c

@@ -230,18 +230,7 @@ ws_cleanup (void)
 static void
 ws_hangup (const char *reason)
 {
-  /* Whether we arrange our own version of opt.lfilename here.  */
+  fprintf (stderr, _("Continuing in background.\n"));
-  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);
-
   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;
-  int changedp;
+  int logfile_changed;
   char lfilename[MAX_PATH + 1];
 };
 
@@ -300,15 +289,19 @@ fake_fork_child (void)
 
   event = info->event;
 
+  info->logfile_changed = 0;
   if (!opt.lfilename)
     {
-      opt.lfilename = unique_name (DEFAULT_LOGFILE, 0);
+      /* See utils:fork_to_background for explanation. */
-      info->changedp = 1;
+      FILE *new_log_fp = unique_create (DEFAULT_LOGFILE, 0, &opt.lfilename);
-      strncpy (info->lfilename, opt.lfilename, sizeof (info->lfilename));
+      if (new_log_fp)
-      info->lfilename[sizeof (info->lfilename) - 1] = '\0';
+	{
+	  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);
@@ -422,7 +415,7 @@ fake_fork (void)
     }
 
   /* Ensure string is properly terminated.  */
-  if (info->changedp &&
+  if (info->logfile_changed &&
       !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);
-  if (info->changedp)
+  if (info->logfile_changed)
     printf (_("Output will be written to `%s'.\n"), info->lfilename);
 
   UnmapViewOfFile (info);

src/utils.c

@@ -283,12 +283,21 @@ fork_to_background (void)
 {
   pid_t pid;
   /* Whether we arrange our own version of opt.lfilename here.  */
-  int changedp = 0;
+  int logfile_changed = 0;
 
   if (!opt.lfilename)
     {
-      opt.lfilename = unique_name (DEFAULT_LOGFILE, 0);
+      /* We must create the file immediately to avoid either a race
-      changedp = 1;
+	 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)
@@ -301,7 +310,7 @@ fork_to_background (void)
     {
       /* 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()? */
     }
@@ -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
-   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
@@ -457,6 +466,65 @@ unique_name (const char *file, int allow_passthrough)
      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 */
+}
 
 /* Create DIRECTORY.  If some of the pathname components of DIRECTORY
    are missing, create them first.  In case any mkdir() call fails,

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));
+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 *));

src/wget.h

@@ -237,7 +237,7 @@ typedef enum
   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,