From: hniksic Date: Fri, 4 Mar 2005 19:26:18 +0000 (-0800) Subject: [svn] Fix a possible race condition when opening files. X-Git-Tag: v1.13~1267 X-Git-Url: http://sjero.net/git/?p=wget;a=commitdiff_plain;h=50d143f3fefbcb343d4d1968d4f9d0d59178ce3f [svn] Fix a possible race condition when opening files. Published in <87r7j6vy9g.fsf@xemacs.org>. --- diff --git a/src/ChangeLog b/src/ChangeLog index 90a79d4d..277a0edb 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,23 @@ +2005-02-24 Hrvoje Niksic + + * 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 * host.c (lookup_host): Test for AI_ADDRCONFIG directly, instead diff --git a/src/ftp.c b/src/ftp.c index e65f8284..c1e2d002 100644 --- 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); - /* #### 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: diff --git a/src/http.c b/src/http.c index 93f072b5..6bc4f3be 100644 --- 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); - 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: diff --git a/src/log.c b/src/log.c index 96582d52..2511513f 100644 --- a/src/log.c +++ b/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); - 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. */ + 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; } diff --git a/src/mswindows.c b/src/mswindows.c index ca1f46a8..22605a95 100644 --- a/src/mswindows.c +++ b/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. */ - 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 @@ -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); - 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); @@ -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); diff --git a/src/utils.c b/src/utils.c index 2589a233..0fe30bfd 100644 --- 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. */ - int changedp = 0; + int logfile_changed = 0; 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) @@ -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, diff --git a/src/utils.h b/src/utils.h index 08aa5667..d22463ed 100644 --- 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)); +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 *)); diff --git a/src/wget.h b/src/wget.h index c41a6814..1ac6eccb 100644 --- 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, - 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,