diff mbox series

Fix -save-temp leaking files in /tmp and possible data loss in signal handler

Message ID AM6PR03MB5170FC381E5C4AC4C02CD726E4110@AM6PR03MB5170.eurprd03.prod.outlook.com
State New
Headers show
Series Fix -save-temp leaking files in /tmp and possible data loss in signal handler | expand

Commit Message

Bernd Edlinger Feb. 18, 2020, 5:12 p.m. UTC 
Hi,

I noticed that my /tmp directory gets clobbered with many *.ld/*.le files whenever
the test suite runs.  I tracked that down to a bug in the collect2 and lto-wrapper
executable, which happens when -save-temps is used.  As I tunrs out, that these
.ld- and .le-files are no longer used, so I removed them altogether, including
the no longer used dump_ld_file.

But when I looked closer I found that the collect2 also calls the not signal safe
function vfprintf (via notice) and the signal safe unlink from a signal handler,
but since the argument for the unlink is potentially accessed before the file path is
initialized, we may call unlink with memory obtained directly from xmalloc, this might
end in removing arbitrary files on the hard disk, when the collect2 is interrupted
asynchronously by any signal handler.

Therefore thus this bug might even deserve a CVE number.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk.


Thanks
Bernd.

Comments

Richard Biener Feb. 19, 2020, 9:21 a.m. UTC | #1 
On Tue, Feb 18, 2020 at 6:12 PM Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>
> Hi,
>
> I noticed that my /tmp directory gets clobbered with many *.ld/*.le files whenever
> the test suite runs.  I tracked that down to a bug in the collect2 and lto-wrapper
> executable, which happens when -save-temps is used.  As I tunrs out, that these
> .ld- and .le-files are no longer used, so I removed them altogether, including
> the no longer used dump_ld_file.
>
> But when I looked closer I found that the collect2 also calls the not signal safe
> function vfprintf (via notice) and the signal safe unlink from a signal handler,
> but since the argument for the unlink is potentially accessed before the file path is
> initialized, we may call unlink with memory obtained directly from xmalloc, this might
> end in removing arbitrary files on the hard disk, when the collect2 is interrupted
> asynchronously by any signal handler.
>
> Therefore thus this bug might even deserve a CVE number.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk.

The collect2 parts are OK for trunk.  Please omit the lto-wrapper change though,
you mention lto-wrapper has a bug above but I see no connection to creating
a new temporary file there.

Thanks,
Richard.

>
> Thanks
> Bernd.
diff mbox series

Patch

From 0fc0c1453c59eb8709d56f7d817caa111eb6eeb7 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Mon, 17 Feb 2020 17:40:07 +0100
Subject: [PATCH] Fix -save-temp leaking files in /tmp

And avoid signal handler calling signal unsafe functions,
and/or calling unlink with uninitialized memory pointer.

2020-02-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* collect2.c (c_file, o_file): Make const again.
	(ldout,lderrout, dump_ld_file): Remove.
	(tool_cleanup): Avoid calling not signal-safe functions.
	(maybe_run_lto_and_relink): Avoid possible signal handler
	access to unintialzed memory (lto_o_files).
	(main): Avoid leaking temp files in $TMPDIR.
	Initialize c_file/o_file with concat, which avoids exposing
	uninitialized memory to signal handler, which calls unlink(!).
	Avoid calling maybe_unlink when the main function returns,
	since the atexit handler is already doing this.
	* collect2.h (dump_ld_file, ldout, lderrout): Remove.
	* lto-wrapper.c (run_gcc): Create an lto tmpdir
	and use it when -save-temps is used.
---
 gcc/collect2.c    | 130 +++++++-----------------------------------------------
 gcc/collect2.h    |   4 --
 gcc/lto-wrapper.c |  11 +++++
 3 files changed, 27 insertions(+), 118 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 502d629..f7d9f10 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -205,14 +205,12 @@  bool helpflag;			/* true if --help */
 static int shared_obj;			/* true if -shared */
 static int static_obj;			/* true if -static */
 
-static char *c_file;		/* <xxx>.c for constructor/destructor list.  */
-static char *o_file;		/* <xxx>.o for constructor/destructor list.  */
+static const char *c_file;		/* <xxx>.c for constructor/destructor list.  */
+static const char *o_file;		/* <xxx>.o for constructor/destructor list.  */
 #ifdef COLLECT_EXPORT_LIST
 static const char *export_file;		/* <xxx>.x for AIX export list.  */
 #endif
 static char **lto_o_files;		/* Output files for LTO.  */
-const char *ldout;			/* File for ld stdout.  */
-const char *lderrout;			/* File for ld stderr.  */
 static const char *output_file;		/* Output file for ld.  */
 static const char *nm_file_name;	/* pathname of nm */
 #ifdef LDD_SUFFIX
@@ -384,6 +382,10 @@  static void scan_prog_file (const char *, scanpass, scanfilter);
 void
 tool_cleanup (bool from_signal)
 {
+  /* maybe_unlink may call notice, which is not signal safe.  */
+  if (from_signal)
+    verbose = false;
+
   if (c_file != 0 && c_file[0])
     maybe_unlink (c_file);
 
@@ -397,20 +399,6 @@  tool_cleanup (bool from_signal)
 
   if (lto_o_files)
     maybe_unlink_list (lto_o_files);
-
-  if (ldout != 0 && ldout[0])
-    {
-      if (!from_signal)
-	dump_ld_file (ldout, stdout);
-      maybe_unlink (ldout);
-    }
-
-  if (lderrout != 0 && lderrout[0])
-    {
-      if (!from_signal)
-	dump_ld_file (lderrout, stderr);
-      maybe_unlink (lderrout);
-    }
 }
 
 static void
@@ -476,77 +464,6 @@  extract_string (const char **pp)
   return XOBFINISH (&temporary_obstack, char *);
 }
 
-void
-dump_ld_file (const char *name, FILE *to)
-{
-  FILE *stream = fopen (name, "r");
-
-  if (stream == 0)
-    return;
-  while (1)
-    {
-      int c;
-      while (c = getc (stream),
-	     c != EOF && (ISIDNUM (c) || c == '$' || c == '.'))
-	obstack_1grow (&temporary_obstack, c);
-      if (obstack_object_size (&temporary_obstack) > 0)
-	{
-	  const char *word, *p;
-	  char *result;
-	  obstack_1grow (&temporary_obstack, '\0');
-	  word = XOBFINISH (&temporary_obstack, const char *);
-
-	  if (*word == '.')
-	    ++word, putc ('.', to);
-	  p = word;
-	  if (!strncmp (p, USER_LABEL_PREFIX, strlen (USER_LABEL_PREFIX)))
-	    p += strlen (USER_LABEL_PREFIX);
-
-#ifdef HAVE_LD_DEMANGLE
-	  result = 0;
-#else
-	  if (no_demangle)
-	    result = 0;
-	  else
-	    result = cplus_demangle (p, DMGL_PARAMS | DMGL_ANSI | DMGL_VERBOSE);
-#endif
-
-	  if (result)
-	    {
-	      int diff;
-	      fputs (result, to);
-
-	      diff = strlen (word) - strlen (result);
-	      while (diff > 0 && c == ' ')
-		--diff, putc (' ', to);
-	      if (diff < 0 && c == ' ')
-		{
-		  while (diff < 0 && c == ' ')
-		    ++diff, c = getc (stream);
-		  if (!ISSPACE (c))
-		    {
-		      /* Make sure we output at least one space, or
-			 the demangled symbol name will run into
-			 whatever text follows.  */
-		      putc (' ', to);
-		    }
-		}
-
-	      free (result);
-	    }
-	  else
-	    fputs (word, to);
-
-	  fflush (to);
-	  obstack_free (&temporary_obstack, temporary_firstobj);
-	}
-      if (c == EOF)
-	break;
-      putc (c, to);
-    }
-  fclose (stream);
-}
-
 /* Return the kind of symbol denoted by name S.  */
 
 static symkind
@@ -744,7 +661,10 @@  maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
 	      ++num_files;
 	  }
 
-	lto_o_files = XNEWVEC (char *, num_files + 1);
+	/* signal handler may access uninitialized memory
+	   and delete whatever it points to, if lto_o_files
+	   is not allocatted with calloc.  */
+	lto_o_files = XCNEWVEC (char *, num_files + 1);
 	lto_o_files[num_files] = NULL;
 	start = XOBFINISH (&temporary_obstack, char *);
 	for (i = 0; i < num_files; ++i)
@@ -1262,27 +1182,19 @@  main (int argc, char **argv)
   /* Make temp file names.  */
   if (save_temps)
     {
-      c_file = (char *) xmalloc (strlen (output_file)
-				  + sizeof (".cdtor.c") + 1);
-      strcpy (c_file, output_file);
-      strcat (c_file, ".cdtor.c");
-      o_file = (char *) xmalloc (strlen (output_file)
-				  + sizeof (".cdtor.o") + 1);
-      strcpy (o_file, output_file);
-      strcat (o_file, ".cdtor.o");
+      c_file = concat (output_file, ".cdtor.c", NULL);
+      o_file = concat (output_file, ".cdtor.o", NULL);
+#ifdef COLLECT_EXPORT_LIST
+      export_file = concat (output_file, ".x", NULL);
+#endif
     }
   else
     {
       c_file = make_temp_file (".cdtor.c");
       o_file = make_temp_file (".cdtor.o");
-    }
 #ifdef COLLECT_EXPORT_LIST
-  export_file = make_temp_file (".x");
+      export_file = make_temp_file (".x");
 #endif
-  if (!debug)
-    {
-      ldout = make_temp_file (".ld");
-      lderrout = make_temp_file (".le");
     }
   /* Build the command line to compile the ctor/dtor list.  */
   *c_ptr++ = c_file_name;
@@ -1811,9 +1723,6 @@  main (int argc, char **argv)
       maybe_unlink (export_file);
 #endif
       post_ld_pass (/*temp_file*/false);
-
-      maybe_unlink (c_file);
-      maybe_unlink (o_file);
       return 0;
     }
 
@@ -1912,13 +1821,6 @@  main (int argc, char **argv)
   scan_prog_file (output_file, PASS_SECOND, SCAN_ALL);
 #endif
 
-  maybe_unlink (c_file);
-  maybe_unlink (o_file);
-
-#ifdef COLLECT_EXPORT_LIST
-  maybe_unlink (export_file);
-#endif
-
   return 0;
 }
 
diff --git a/gcc/collect2.h b/gcc/collect2.h
index ab5fcdf..aa8a03e 100644
--- a/gcc/collect2.h
+++ b/gcc/collect2.h
@@ -25,12 +25,8 @@  extern struct pex_obj *collect_execute (const char *, char **, const char *,
 
 extern int collect_wait (const char *, struct pex_obj *);
 
-extern void dump_ld_file (const char *, FILE *);
-
 extern int file_exists (const char *);
 
-extern const char *ldout;
-extern const char *lderrout;
 extern const char *c_file_name;
 extern struct obstack temporary_obstack;
 extern char *temporary_firstobj;
diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index fe8f292..fdc9565 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1423,6 +1423,17 @@  run_gcc (unsigned argc, char *argv[])
       fputc ('\n', stderr);
     }
 
+  if (save_temps)
+    {
+      char *tmpdir = concat (linker_output ? linker_output : "a.out",
+			     ".lto_tmpdir", NULL);
+      /* Make directory if necessary, but expect no race here.  */
+      if (access (tmpdir, F_OK) == 0
+	  || mkdir (tmpdir, S_IRWXU | S_IRWXG | S_IRWXO) == 0)
+	setenv ("TMPDIR", tmpdir, 1);
+      free (tmpdir);
+    }
+
   if (linker_output_rel)
     no_partition = true;
 
-- 
1.9.1