[patch] temporary file handling

[patch] temporary file handling

[Vasiliy Kulikov]

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

Hi,

This patch fixes some temporary file handling issues:

  * mktemp() returns an empty string on error, not NULL.  The code
    wrongly checked for the latter, but not for the former.  This is now
    corrected.
  * make_tempname() and make_tempdir() didn't free the pointer returned
    by template_in_dir() if an error occurred in a subsequent call, such
    as to mkstemp() or open().  This might have made no difference since
    binutils programs would typically call fatal() if make_temp*() fails,
    however it makes more of a difference along with the change below:
  * make_tempname() and make_tempdir() on systems without mkstemp()
    and mkdtemp(), respectively, would abort immediately if the filename
    is taken (by another process) between the calls to mktemp() and
    open()/mkdir().  This patch improves the code to emulate mkstemp()
    and mkdtemp() more closely by retrying the mktemp() and open()/mkdir()
    in a loop (much like what mkstemp() and mkdtemp() typically do).
  * choose_temp_base() is not a safe API since a filename made from a
  * unique
    filename with a suffix appended is not guaranteed unique anymore, and
    since the filename was not reserved anyway.  The patch replaces the uses
    of choose_temp_base() with those of make_temp_file().
  * tmpnam() is a racy API.  Libiberty's implementation is buggy even
    without the race: it checks file existence with fopen(file, "r").
    If file is accessible only for write, then fopen() fails and tmpnam()
    returns the name of an already existing file.  tic54x_mlib() from
    tc-tic54x.c called fopen(name, "w") with the name returned from tmpnam().
    So a file with no read permissions could be overwritten, including over a
    (sym)link.  The patch corrects this by using make_temp_file() instead.

The tempbase field from struct pex_obj was needed only for
choose_temp_base(), so I've deleted it.  pexecute() and some functions
don't need the tempbase argument anymore.  However, in this patch I've
only commented out some code to ignore tempbase.  You might want to fix
this in a cleaner but more invasive manner.

I've tested the patch by rebuilding the Openwall GNU/*/Linux userland with
the patched binutils in place (twice on each of i686 and x86_64, with the
second rebuild using development/build tools built the first time).

This patch was originally created for H. J. Lu's version of binutils,
but it looks OK for GNU's version.


Also you can find this patch (or possible revisions of it) here:
http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/binutils/


Thanks,

-- 
Vasiliy

[-- Attachment #2: binutils-2.20.51.0.11-owl-tmp.diff --]
[-- Type: text/x-diff, Size: 11068 bytes --]

diff -uNrp binutils-2.20.51.0.11.old/binutils/bucomm.c binutils-2.20.51.0.11/binutils/bucomm.c
--- binutils-2.20.51.0.11.old/binutils/bucomm.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/binutils/bucomm.c	2010-10-04 19:11:31.000000000 +0000
@@ -491,19 +491,33 @@ template_in_dir (const char *path)
 char *
 make_tempname (char *filename)
 {
-  char *tmpname = template_in_dir (filename);
+  char *tmpname;
   int fd;
 
 #ifdef HAVE_MKSTEMP
+  tmpname = template_in_dir (filename);
   fd = mkstemp (tmpname);
 #else
-  tmpname = mktemp (tmpname);
-  if (tmpname == NULL)
-    return NULL;
-  fd = open (tmpname, O_RDWR | O_CREAT | O_EXCL, 0600);
+  int i;
+  fd = -1;
+  for (i = 0; i < 1000; i++)
+    {
+      tmpname = template_in_dir (filename);
+      (void) mktemp (tmpname);
+      if (tmpname[0] == 0)
+        break;
+      fd = open (tmpname, O_RDWR | O_CREAT | O_EXCL, 0600);
+      if (fd != -1)
+        break;
+      free (tmpname);
+      tmpname = NULL;
+    }
 #endif
   if (fd == -1)
-    return NULL;
+    {
+      free (tmpname);
+      return NULL;
+    }
   close (fd);
   return tmpname;
 }
@@ -514,23 +528,36 @@ make_tempname (char *filename)
 char *
 make_tempdir (char *filename)
 {
-  char *tmpname = template_in_dir (filename);
-
+  char *tmpname, *res;
 #ifdef HAVE_MKDTEMP
-  return mkdtemp (tmpname);
+  tmpname = template_in_dir (filename);
+  res = mkdtemp (tmpname);
 #else
-  tmpname = mktemp (tmpname);
-  if (tmpname == NULL)
-    return NULL;
+  int i;
+  res = NULL;
+  for (i = 0; i < 1000; i++)
+    {
+      tmpname = template_in_dir (filename);
+      (void) mktemp (tmpname);
+      if (tmpname[0] == 0)
+        break;
+
 #if defined (_WIN32) && !defined (__CYGWIN32__)
-  if (mkdir (tmpname) != 0)
-    return NULL;
+      if (mkdir (tmpname) == 0)
 #else
-  if (mkdir (tmpname, 0700) != 0)
-    return NULL;
+      if (mkdir (tmpname, 0700) == 0)
 #endif
-  return tmpname;
+        {
+          res = tmpname;
+          break;
+        }
+      free (tmpname);
+      tmpname = NULL;
+    }
 #endif
+  if (res == NULL)
+    free (tmpname);
+  return res;
 }
 
 /* Parse a string into a VMA, with a fatal error if it can't be
diff -uNrp binutils-2.20.51.0.11.old/binutils/dlltool.c binutils-2.20.51.0.11/binutils/dlltool.c
--- binutils-2.20.51.0.11.old/binutils/dlltool.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/binutils/dlltool.c	2010-10-01 12:59:09.000000000 +0000
@@ -1297,7 +1297,6 @@ run (const char *what, char *args)
   int i;
   const char **argv;
   char *errmsg_fmt, *errmsg_arg;
-  char *temp_base = choose_temp_base ();
 
   inform ("run: %s %s", what, args);
 
@@ -1324,7 +1323,7 @@ run (const char *what, char *args)
     }
   argv[i++] = NULL;
 
-  pid = pexecute (argv[0], (char * const *) argv, program_name, temp_base,
+  pid = pexecute (argv[0], (char * const *) argv, program_name, 0,
 		  &errmsg_fmt, &errmsg_arg, PEXECUTE_ONE | PEXECUTE_SEARCH);
 
   if (pid == -1)
diff -uNrp binutils-2.20.51.0.11.old/binutils/dllwrap.c binutils-2.20.51.0.11/binutils/dllwrap.c
--- binutils-2.20.51.0.11.old/binutils/dllwrap.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/binutils/dllwrap.c	2010-10-01 12:59:26.000000000 +0000
@@ -358,7 +358,6 @@ run (const char *what, char *args)
   int i;
   const char **argv;
   char *errmsg_fmt, *errmsg_arg;
-  char *temp_base = choose_temp_base ();
   int in_quote;
   char sep;
 
@@ -397,7 +396,7 @@ run (const char *what, char *args)
   if (dry_run)
     return 0;
 
-  pid = pexecute (argv[0], (char * const *) argv, prog_name, temp_base,
+  pid = pexecute (argv[0], (char * const *) argv, prog_name, 0,
 		  &errmsg_fmt, &errmsg_arg, PEXECUTE_ONE | PEXECUTE_SEARCH);
 
   if (pid == -1)
@@ -823,13 +822,7 @@ main (int argc, char **argv)
 
   if (! def_file_seen)
     {
-      char *fileprefix = choose_temp_base ();
-
-      def_file_name = (char *) xmalloc (strlen (fileprefix) + 5);
-      sprintf (def_file_name, "%s.def",
-	       (dontdeltemps) ? mybasename (fileprefix) : fileprefix);
-      delete_def_file = 1;
-      free (fileprefix);
+      def_file_name = make_temp_file(".def");
       delete_def_file = 1;
       warn (_("no export definition file provided.\n\
 Creating one, but that may not be what you want"));
@@ -1036,12 +1029,8 @@ Creating one, but that may not be what y
 
   if (! base_file_name)
     {
-      char *fileprefix = choose_temp_base ();
-      base_file_name = (char *) xmalloc (strlen (fileprefix) + 6);
-      sprintf (base_file_name, "%s.base",
-	       (dontdeltemps) ? mybasename (fileprefix) : fileprefix);
+      base_file_name = make_temp_file(".base");
       delete_base_file = 1;
-      free (fileprefix);
     }
 
   {
diff -uNrp binutils-2.20.51.0.11.old/binutils/resrc.c binutils-2.20.51.0.11/binutils/resrc.c
--- binutils-2.20.51.0.11.old/binutils/resrc.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/binutils/resrc.c	2010-10-01 12:59:41.000000000 +0000
@@ -207,7 +207,6 @@ run_cmd (char *cmd, const char *redir)
   int i;
   const char **argv;
   char *errmsg_fmt, *errmsg_arg;
-  char *temp_base = choose_temp_base ();
   int in_quote;
   char sep;
   int redir_handle = -1;
@@ -270,7 +269,7 @@ run_cmd (char *cmd, const char *redir)
   /* Redirect stdout to our output file.  */
   dup2 (redir_handle, STDOUT_FILENO);
 
-  pid = pexecute (argv[0], (char * const *) argv, program_name, temp_base,
+  pid = pexecute (argv[0], (char * const *) argv, program_name, 0,
 		  &errmsg_fmt, &errmsg_arg, PEXECUTE_ONE | PEXECUTE_SEARCH);
 
   /* Restore stdout to its previous setting.  */
@@ -318,12 +317,7 @@ open_input_stream (char *cmd)
 {
   if (istream_type == ISTREAM_FILE)
     {
-      char *fileprefix;
-
-      fileprefix = choose_temp_base ();
-      cpp_temp_file = (char *) xmalloc (strlen (fileprefix) + 5);
-      sprintf (cpp_temp_file, "%s.irc", fileprefix);
-      free (fileprefix);
+      cpp_temp_file = make_temp_file(".irc");
 
       if (run_cmd (cmd, cpp_temp_file))
 	fatal (_("can't execute `%s': %s"), cmd, strerror (errno));
diff -uNrp binutils-2.20.51.0.11.old/gas/config/tc-tic54x.c binutils-2.20.51.0.11/gas/config/tc-tic54x.c
--- binutils-2.20.51.0.11.old/gas/config/tc-tic54x.c	2010-09-30 16:55:34.000000000 +0000
+++ binutils-2.20.51.0.11/gas/config/tc-tic54x.c	2010-10-01 13:26:34.000000000 +0000
@@ -2365,7 +2365,7 @@ tic54x_mlib (int ignore ATTRIBUTE_UNUSED
       /* Get a size at least as big as the archive member.  */
       bfd_size_type size = bfd_get_size (mbfd);
       char *buf = xmalloc (size);
-      char *fname = tmpnam (NULL);
+      char *fname = make_temp_file(NULL);
       FILE *ftmp;
 
       /* We're not sure how big it is, but it will be smaller than "size".  */
diff -uNrp binutils-2.20.51.0.11.old/include/libiberty.h binutils-2.20.51.0.11/include/libiberty.h
--- binutils-2.20.51.0.11.old/include/libiberty.h	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/include/libiberty.h	2010-10-01 12:26:10.000000000 +0000
@@ -220,9 +220,11 @@ extern char *make_relative_prefix (const
 extern char *make_relative_prefix_ignore_links (const char *, const char *,
 						const char *) ATTRIBUTE_MALLOC;
 
+#if 0
 /* Choose a temporary directory to use for scratch files.  */
 
 extern char *choose_temp_base (void) ATTRIBUTE_MALLOC;
+#endif
 
 /* Return a temporary file name or NULL if unable to create one.  */
 
diff -uNrp binutils-2.20.51.0.11.old/libiberty/choose-temp.c binutils-2.20.51.0.11/libiberty/choose-temp.c
--- binutils-2.20.51.0.11.old/libiberty/choose-temp.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/libiberty/choose-temp.c	2010-10-01 13:00:24.000000000 +0000
@@ -57,6 +57,7 @@ not recommended.
 
 */
 
+#if 0
 char *
 choose_temp_base (void)
 {
@@ -73,3 +74,4 @@ choose_temp_base (void)
     abort ();
   return temp_filename;
 }
+#endif
diff -uNrp binutils-2.20.51.0.11.old/libiberty/pex-common.c binutils-2.20.51.0.11/libiberty/pex-common.c
--- binutils-2.20.51.0.11.old/libiberty/pex-common.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/libiberty/pex-common.c	2010-10-01 13:02:25.000000000 +0000
@@ -58,7 +58,7 @@ pex_init_common (int flags, const char *
   obj = XNEW (struct pex_obj);
   obj->flags = flags;
   obj->pname = pname;
-  obj->tempbase = tempbase;
+  (void) tempbase;
   obj->next_input = STDIN_FILE_NO;
   obj->next_input_name = NULL;
   obj->next_input_name_allocated = 0;
@@ -94,6 +94,7 @@ pex_add_remove (struct pex_obj *obj, con
   obj->remove[obj->remove_count - 1] = add;
 }
 
+#if 0
 /* Generate a temporary file name based on OBJ, FLAGS, and NAME.
    Return NULL if we were unable to reserve a temporary filename.
 
@@ -143,6 +144,7 @@ temp_file (struct pex_obj *obj, int flag
 
   return name;
 }
+#endif
 
 
 /* As for pex_run (), but permits the environment for the child process
@@ -222,14 +224,14 @@ pex_run_in_environment (struct pex_obj *
 	out = STDOUT_FILE_NO;
       else if ((flags & PEX_SUFFIX) != 0)
 	{
-	  outname = concat (obj->tempbase, outname, NULL);
+	  outname = make_temp_file(outname);
 	  outname_allocated = 1;
 	}
       obj->next_input = -1;
     }
   else if ((obj->flags & PEX_USE_PIPES) == 0)
     {
-      outname = temp_file (obj, flags, outname);
+      outname = make_temp_file(outname);
       if (! outname)
         {
           *err = 0;
@@ -391,7 +393,7 @@ pex_input_file (struct pex_obj *obj, int
       return NULL;
     }
 
-  name = temp_file (obj, flags, name);
+  name = make_temp_file(name);
   if (! name)
     return NULL;
 
diff -uNrp binutils-2.20.51.0.11.old/libiberty/pex-common.h binutils-2.20.51.0.11/libiberty/pex-common.h
--- binutils-2.20.51.0.11.old/libiberty/pex-common.h	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/libiberty/pex-common.h	2010-10-01 12:31:29.000000000 +0000
@@ -57,8 +57,10 @@ struct pex_obj
   int flags;
   /* Name of calling program, for error messages.  */
   const char *pname;
+#if 0
   /* Base name to use for temporary files.  */
   const char *tempbase;
+#endif
   /* Pipe to use as stdin for next process.  */
   int next_input;
   /* File name to use as stdin for next process.  */
diff -uNrp binutils-2.20.51.0.11.old/libiberty/pex-msdos.c binutils-2.20.51.0.11/libiberty/pex-msdos.c
--- binutils-2.20.51.0.11.old/libiberty/pex-msdos.c	2010-09-30 16:55:35.000000000 +0000
+++ binutils-2.20.51.0.11/libiberty/pex-msdos.c	2010-10-01 13:03:10.000000000 +0000
@@ -161,7 +161,6 @@ pex_msdos_exec_child (struct pex_obj *ob
 {
   struct pex_msdos *ms;
   char *temp_base;
-  int temp_base_allocated;
   char *rf;
   int inindex;
   char *infile;
@@ -177,6 +176,7 @@ pex_msdos_exec_child (struct pex_obj *ob
   /* FIXME: I don't know how to redirect stderr, so we ignore ERRDES
      and PEX_STDERR_TO_STDOUT.  */
 
+#if 0
   temp_base = obj->temp_base;
   if (temp_base != NULL)
     temp_base_allocated = 0;
@@ -190,6 +190,8 @@ pex_msdos_exec_child (struct pex_obj *ob
 
   if (temp_base_allocated)
     free (temp_base);
+#endif
+  rf = make_temp_file(".gp");
 
   if (in == STDIN_FILE_NO)
     {

Re: [patch] temporary file handling

[Alan Modra]

On Sun, Oct 10, 2010 at 01:42:58PM +0400, Vasiliy Kulikov wrote:
> diff -uNrp binutils-2.20.51.0.11.old/binutils/bucomm.c binutils-2.20.51.0.11/binutils/bucomm.c
> --- binutils-2.20.51.0.11.old/binutils/bucomm.c	2010-09-30 16:55:35.000000000 +0000
> +++ binutils-2.20.51.0.11/binutils/bucomm.c	2010-10-04 19:11:31.000000000 +0000
> @@ -491,19 +491,33 @@ template_in_dir (const char *path)
>  char *
>  make_tempname (char *filename)
>  {
> -  char *tmpname = template_in_dir (filename);
> +  char *tmpname;
>    int fd;
>  
>  #ifdef HAVE_MKSTEMP
> +  tmpname = template_in_dir (filename);
>    fd = mkstemp (tmpname);
>  #else
> -  tmpname = mktemp (tmpname);
> -  if (tmpname == NULL)
> -    return NULL;
> -  fd = open (tmpname, O_RDWR | O_CREAT | O_EXCL, 0600);
> +  int i;
> +  fd = -1;
> +  for (i = 0; i < 1000; i++)
> +    {
> +      tmpname = template_in_dir (filename);
> +      (void) mktemp (tmpname);
> +      if (tmpname[0] == 0)
> +        break;
> +      fd = open (tmpname, O_RDWR | O_CREAT | O_EXCL, 0600);
> +      if (fd != -1)
> +        break;
> +      free (tmpname);
> +      tmpname = NULL;
> +    }
>  #endif
>    if (fd == -1)
> -    return NULL;
> +    {
> +      free (tmpname);

Possible free(NULL) here and in make_tempdir, which may crash on some
older implementations of free.  We seem to worry about that
possibility elsewhere in binutils.

As the libiberty README says, libiberty patch should go to
gcc-patches@gcc.gnu.org, and maintainers of gcc and binutils generally
don't like #if 0 to remove code.  Even if this is fixed, I suspect
your patch will be rejected due to losing the capability to specify a
temp dir via the temp_base arg of pexecute.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] temporary file handling

[Solar Designer]

Alan,

Thank you for your review!

On Mon, Oct 11, 2010 at 01:38:42PM +1030, Alan Modra wrote:
> Possible free(NULL) here and in make_tempdir, which may crash on some
> older implementations of free.  We seem to worry about that
> possibility elsewhere in binutils.

OK.  With this fixed, would you accept the changes to binutils/bucomm.c?
While unimportant for modern systems that have mkstemp() and mkdtemp(),
these are rather obvious fixes for older systems that only have mktemp().
For example, this:

  tmpname = mktemp (tmpname);
  if (tmpname == NULL)
    return NULL;

was obviously wrong because mktemp() returns an empty string rather than
NULL on error.

Would you like Vasiliy to fix the free(NULL) issue, add a
binutils/ChangeLog entry, and submit just this part of the patch?

> As the libiberty README says, libiberty patch should go to
> gcc-patches@gcc.gnu.org, and maintainers of gcc and binutils generally
> don't like #if 0 to remove code.  Even if this is fixed, I suspect
> your patch will be rejected due to losing the capability to specify a
> temp dir via the temp_base arg of pexecute.

Thank you for explaining this and sharing your opinion.  We'll consider
revising/submitting this part of the patch separately and to the proper
place.

Alexander Peslyak <solar at openwall.com>
http://www.openwall.com - bringing security into open computing environments

Re: [patch] temporary file handling

[Alan Modra]

On Mon, Oct 11, 2010 at 10:27:01PM +0400, Solar Designer wrote:
> Would you like Vasiliy to fix the free(NULL) issue, add a
> binutils/ChangeLog entry, and submit just this part of the patch?

Yes please.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] temporary file handling

[Tristan Gingold]

On Oct 11, 2010, at 5:08 AM, Alan Modra wrote:
> 
> Possible free(NULL) here and in make_tempdir, which may crash on some
> older implementations of free.  We seem to worry about that
> possibility elsewhere in binutils.

Side question:  we currently assume ISO-C (aka ANSI-C) compiler.  Don't we also assume ANSI-C library ?

Tristan.

Re: [patch] temporary file handling

[Alan Modra]

On Tue, Oct 12, 2010 at 08:45:48AM +0200, Tristan Gingold wrote:
> 
> On Oct 11, 2010, at 5:08 AM, Alan Modra wrote:
> > 
> > Possible free(NULL) here and in make_tempdir, which may crash on some
> > older implementations of free.  We seem to worry about that
> > possibility elsewhere in binutils.
> 
> Side question:  we currently assume ISO-C (aka ANSI-C) compiler.  Don't we also assume ANSI-C library ?

I didn't want to set that policy, which is why I mentioned free(0).
Nowadays, it probably would be quite a reasonable requirement.

-- 
Alan Modra
Australia Development Lab, IBM