public inbox for libc-hacker@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Use alloca in execvp if possible
@ 2007-01-03 13:15 Jakub Jelinek
  2007-01-03 23:03 ` Ulrich Drepper
  0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2007-01-03 13:15 UTC (permalink / raw)
  To: Ulrich Drepper; +Cc: Glibc hackers

Hi!

If execvp is used after vfork (which should be possible as execvp is one of
the exec* family functions and POSIX allows them to be called after vfork),
it leaks memory in the parent process (as the memory is only freed if execve
fails and the address space is shared with the parent).
The following patch tries to use alloca when possible (should cover most
usual cases), alternatively we could say add a __thread vfork_mem pointer
and wrappers around malloc/free/realloc which execvp/execl* etc. could use.
The wrappers would allocate extra pointer at the beginning of the memory
blocks and chain all such allocations together.  When vfork returns in
parent, it would walk this chain and free all the buffers mentioned there.
vfork is not async signal safe, so it ought to be possible, but probably
overkill.

2007-01-03  Jakub Jelinek  <jakub@redhat.com>

	* posix/execvp.c: Include alloca.h.
	(allocate_scripts_argv): Renamed to...
	(scripts_argv): ... this.  Don't allocate buffer here nor count
	arguments.
	(execvp): Use alloca if possible.
	* posix/Makefile: Add rules to build and run tst-vfork3 test.
	* posix/tst-vfork3.c: New test.

--- libc/posix/execvp.c.jj	2005-07-24 23:38:43.000000000 +0200
+++ libc/posix/execvp.c	2007-01-03 13:49:28.000000000 +0100
@@ -1,4 +1,5 @@
-/* Copyright (C) 1991,92,1995-99,2002,2004,2005 Free Software Foundation, Inc.
+/* Copyright (C) 1991,92, 1995-99, 2002, 2004, 2005, 2007
+   Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -16,6 +17,7 @@
    Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
    02111-1307 USA.  */
 
+#include <alloca.h>
 #include <unistd.h>
 #include <stdarg.h>
 #include <stdbool.h>
@@ -27,29 +29,18 @@
 
 /* The file is accessible but it is not an executable file.  Invoke
    the shell to interpret it as a script.  */
-static char **
+static void
 internal_function
-allocate_scripts_argv (const char *file, char *const argv[])
+scripts_argv (const char *file, char *const argv[], int argc, char **new_argv)
 {
-  /* Count the arguments.  */
-  int argc = 0;
-  while (argv[argc++])
-    ;
-
   /* Construct an argument list for the shell.  */
-  char **new_argv = (char **) malloc ((argc + 1) * sizeof (char *));
-  if (new_argv != NULL)
+  new_argv[0] = (char *) _PATH_BSHELL;
+  new_argv[1] = (char *) file;
+  while (argc > 1)
     {
-      new_argv[0] = (char *) _PATH_BSHELL;
-      new_argv[1] = (char *) file;
-      while (argc > 1)
-	{
-	  new_argv[argc] = argv[argc - 1];
-	  --argc;
-	}
+      new_argv[argc] = argv[argc - 1];
+      --argc;
     }
-
-  return new_argv;
 }
 
 
@@ -67,8 +58,6 @@ execvp (file, argv)
       return -1;
     }
 
-  char **script_argv = NULL;
-
   if (strchr (file, '/') != NULL)
     {
       /* Don't search when it contains a slash.  */
@@ -76,46 +65,71 @@ execvp (file, argv)
 
       if (errno == ENOEXEC)
 	{
-	  script_argv = allocate_scripts_argv (file, argv);
+	  /* Count the arguments.  */
+	  int argc = 0;
+	  while (argv[argc++])
+	    ;
+	  size_t len = (argc + 1) * sizeof (char *);
+	  char **script_argv;
+	  void *ptr = NULL;
+	  if (__libc_use_alloca (len))
+	    script_argv = alloca (len);
+	  else
+	    script_argv = ptr = malloc (len);
+
 	  if (script_argv != NULL)
 	    {
+	      scripts_argv (file, argv, argc, script_argv);
 	      __execve (script_argv[0], script_argv, __environ);
 
-	      free (script_argv);
+	      free (ptr);
 	    }
 	}
     }
   else
     {
+      size_t pathlen;
+      size_t alloclen = 0;
       char *path = getenv ("PATH");
-      char *path_malloc = NULL;
+      if (path == NULL)
+	{
+	  pathlen = confstr (_CS_PATH, (char *) NULL, 0);
+	  alloclen = pathlen + 1;
+	}
+      else
+	pathlen = strlen (path);
+
+      size_t len = strlen (file) + 1;
+      alloclen += pathlen + len + 1;
+
+      char *name;
+      char *path_malloc = NULL;
+      if (__libc_use_alloca (alloclen))
+	name = alloca (alloclen);
+      else
+	{
+	  path_malloc = name = malloc (alloclen);
+	  if (name == NULL)
+	    return -1;
+	}
+
       if (path == NULL)
 	{
 	  /* There is no `PATH' in the environment.
 	     The default search path is the current directory
 	     followed by the path `confstr' returns for `_CS_PATH'.  */
-	  size_t len = confstr (_CS_PATH, (char *) NULL, 0);
-	  path = (char *) malloc (1 + len);
-	  if (path == NULL)
-	    return -1;
+	  path = name + pathlen + len + 1;
 	  path[0] = ':';
-	  (void) confstr (_CS_PATH, path + 1, len);
-	  path_malloc = path;
+	  (void) confstr (_CS_PATH, path + 1, pathlen);
 	}
 
-      size_t len = strlen (file) + 1;
-      size_t pathlen = strlen (path);
-      char *name = malloc (pathlen + len + 1);
-      if (name == NULL)
-	{
-	  free (path_malloc);
-	  return -1;
-	}
       /* Copy the file name at the top.  */
       name = (char *) memcpy (name + pathlen + 1, file, len);
       /* And add the slash.  */
       *--name = '/';
 
+      char **script_argv = NULL;
+      void *script_argv_malloc = NULL;
       bool got_eacces = false;
       char *p = path;
       do
@@ -139,7 +153,15 @@ execvp (file, argv)
 	    {
 	      if (script_argv == NULL)
 		{
-		  script_argv = allocate_scripts_argv (startp, argv);
+		  /* Count the arguments.  */
+		  int argc = 0;
+		  while (argv[argc++])
+		    ;
+		  size_t arglen = (argc + 1) * sizeof (char *);
+		  if (__libc_use_alloca (alloclen + arglen))
+		    script_argv = alloca (arglen);
+		  else
+		    script_argv = script_argv_malloc = malloc (arglen);
 		  if (script_argv == NULL)
 		    {
 		      /* A possible EACCES error is not as important as
@@ -147,6 +169,7 @@ execvp (file, argv)
 		      got_eacces = false;
 		      break;
 		    }
+		  scripts_argv (startp, argv, argc, script_argv);
 		}
 
 	      __execve (script_argv[0], script_argv, __environ);
@@ -184,11 +207,10 @@ execvp (file, argv)
       /* We tried every element and none of them worked.  */
       if (got_eacces)
 	/* At least one failure was due to permissions, so report that
-           error.  */
+	   error.  */
 	__set_errno (EACCES);
 
-      free (script_argv);
-      free (name - pathlen);
+      free (script_argv_malloc);
       free (path_malloc);
     }
 
--- libc/posix/Makefile.jj	2006-09-07 15:50:05.000000000 +0200
+++ libc/posix/Makefile	2007-01-03 12:25:12.000000000 +0100
@@ -1,4 +1,4 @@
-# Copyright (C) 1991-1999, 2000-2005, 2006 Free Software Foundation, Inc.
+# Copyright (C) 1991-1999, 2000-2006, 2007 Free Software Foundation, Inc.
 # This file is part of the GNU C Library.
 
 # The GNU C Library is free software; you can redistribute it and/or
@@ -83,7 +83,7 @@ tests		:= tstgetopt testfnm runtests run
 		   bug-regex21 bug-regex22 bug-regex23 bug-regex24 \
 		   bug-regex25 bug-regex26 tst-nice tst-nanosleep tst-regex2 \
 		   transbug tst-rxspencer tst-pcre tst-boost \
-		   bug-ga1 tst-vfork1 tst-vfork2 tst-waitid \
+		   bug-ga1 tst-vfork1 tst-vfork2 tst-vfork3 tst-waitid \
 		   tst-getaddrinfo2 bug-glob1 bug-glob2 tst-sysconf \
 		   tst-execvp1 tst-execvp2 tst-execlp1 tst-execlp2 \
 		   tst-execv1 tst-execv2 tst-execl1 tst-execl2 \
@@ -108,7 +108,8 @@ generated := $(addprefix wordexp-test-re
 	     bug-regex21-mem bug-regex21.mtrace \
 	     tst-rxspencer-mem tst-rxspencer.mtrace tst-getconf.out \
 	     tst-pcre-mem tst-pcre.mtrace tst-boost-mem tst-boost.mtrace \
-	     bug-ga2.mtrace bug-ga2-mem bug-glob2.mtrace bug-glob2-mem
+	     bug-ga2.mtrace bug-ga2-mem bug-glob2.mtrace bug-glob2-mem \
+	     tst-vfork3-mem tst-vfork3.mtrace
 
 include ../Rules
 
@@ -218,7 +219,7 @@ ifeq (no,$(cross-compiling))
 tests: $(objpfx)bug-regex2-mem $(objpfx)bug-regex14-mem \
   $(objpfx)bug-regex21-mem $(objpfx)tst-rxspencer-mem \
   $(objpfx)tst-pcre-mem $(objpfx)tst-boost-mem $(objpfx)tst-getconf.out \
-  $(objpfx)bug-glob2-mem
+  $(objpfx)bug-glob2-mem $(objpfx)tst-vfork3-mem
 xtests: $(objpfx)bug-ga2-mem
 endif
 
@@ -245,6 +246,11 @@ bug-regex21-ENV = MALLOC_TRACE=$(objpfx)
 $(objpfx)bug-regex21-mem: $(objpfx)bug-regex21.out
 	$(common-objpfx)malloc/mtrace $(objpfx)bug-regex21.mtrace > $@
 
+tst-vfork3-ENV = MALLOC_TRACE=$(objpfx)tst-vfork3.mtrace
+
+$(objpfx)tst-vfork3-mem: $(objpfx)tst-vfork3.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-vfork3.mtrace > $@
+
 # tst-rxspencer.mtrace is generated only when run without --utf8
 # option, since otherwise the file has almost 100M and takes very long
 # time to process.
--- libc/posix/tst-vfork3.c.jj	2007-01-03 11:19:07.000000000 +0100
+++ libc/posix/tst-vfork3.c	2007-01-03 12:18:12.000000000 +0100
@@ -0,0 +1,219 @@
+/* Test for vfork functions.
+   Copyright (C) 2007 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Jakub Jelinek <jakub@redhat.com>, 2007.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <mcheck.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+static int do_test (void);
+static void do_prepare (void);
+char *tmpdirname;
+
+#define TEST_FUNCTION do_test ()
+#define PREPARE(argc, argv) do_prepare ()
+#include "../test-skeleton.c"
+
+static int
+do_test (void)
+{
+  mtrace ();
+
+  char *path = getenv ("PATH");
+  if (path == NULL)
+    path = "/bin";
+  char pathbuf [strlen (tmpdirname) + 1 + strlen (path) + 1];
+  strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path);
+  if (setenv ("PATH", pathbuf, 1) < 0)
+    {
+      puts ("setenv failed");
+      return 1;
+    }
+
+  size_t i;
+  char *argv[3] = { "script1.sh", "1", NULL };
+  for (i = 0; i < 5; i++)
+    {
+      pid_t pid = vfork ();
+      if (pid < 0)
+	{
+	  printf ("vfork failed: %m\n");
+	  return 1;
+	}
+      else if (pid == 0)
+	{
+	  execvp ("script1.sh", argv);
+	  _exit (errno);
+	}
+      int status;
+      if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
+	{
+	  puts ("waitpid failed");
+	  return 1;
+	}
+      else if (status != 0)
+	{
+	  printf ("script1.sh failed with status %d\n", status);
+	  return 1;
+	}
+    }
+
+  argv[0] = "script2.sh";
+  argv[1] = "2";
+  for (i = 0; i < 5; i++)
+    {
+      pid_t pid = vfork ();
+      if (pid < 0)
+	{
+	  printf ("vfork failed: %m\n");
+	  return 1;
+	}
+      else if (pid == 0)
+	{
+	  execvp ("script2.sh", argv);
+	  _exit (errno);
+	}
+      int status;
+      if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
+	{
+	  puts ("waitpid failed");
+	  return 1;
+	}
+      else if (status != 0)
+	{
+	  printf ("script2.sh failed with status %d\n", status);
+	  return 1;
+	}
+    }
+
+  for (i = 0; i < 5; i++)
+    {
+      pid_t pid = vfork ();
+      if (pid < 0)
+	{
+	  printf ("vfork failed: %m\n");
+	  return 1;
+	}
+      else if (pid == 0)
+	{
+	  execlp ("script2.sh", "script2.sh", "3", NULL);
+	  _exit (errno);
+	}
+      int status;
+      if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
+	{
+	  puts ("waitpid failed");
+	  return 1;
+	}
+      else if (status != 0)
+	{
+	  printf ("script2.sh failed with status %d\n", status);
+	  return 1;
+	}
+    }
+
+  unsetenv ("PATH");
+  argv[0] = "echo";
+  argv[1] = "script 4";
+  for (i = 0; i < 5; i++)
+    {
+      pid_t pid = vfork ();
+      if (pid < 0)
+	{
+	  printf ("vfork failed: %m\n");
+	  return 1;
+	}
+      else if (pid == 0)
+	{
+	  execvp ("echo", argv);
+	  _exit (errno);
+	}
+      int status;
+      if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
+	{
+	  puts ("waitpid failed");
+	  return 1;
+	}
+      else if (status != 0)
+	{
+	  printf ("echo failed with status %d\n", status);
+	  return 1;
+	}
+    }
+
+  return 0;
+}
+
+static void
+do_prepare (void)
+{
+  size_t len = strlen (test_dir) + sizeof ("/tst-vfork3.XXXXXX");
+  tmpdirname = malloc (len);
+  char *script1 = malloc (len + sizeof "/script1.sh");
+  char *script2 = malloc (len + sizeof "/script2.sh");
+  if (tmpdirname == NULL || script1 == NULL || script2 == NULL)
+    {
+      puts ("out of memory");
+      exit (1);
+    }
+  strcpy (stpcpy (tmpdirname, test_dir), "/tst-vfork3.XXXXXX");
+
+  tmpdirname = mkdtemp (tmpdirname);
+  if (tmpdirname == NULL)
+    {
+      puts ("could not create temporary directory");
+      exit (1);
+    }
+
+  strcpy (stpcpy (script1, tmpdirname), "/script1.sh");
+  strcpy (stpcpy (script2, tmpdirname), "/script2.sh");
+
+  /* Need to make sure tmpdirname is at the end of the linked list.  */
+  add_temp_file (script1);
+  add_temp_file (tmpdirname);
+  add_temp_file (script2);
+
+  const char content1[] = "#!/bin/sh\necho script $1\n";
+  int fd = open (script1, O_WRONLY | O_CREAT, 0700);
+  if (fd < 0
+      || TEMP_FAILURE_RETRY (write (fd, content1, sizeof content1))
+	 != sizeof content1
+      || fchmod (fd, S_IRUSR | S_IXUSR) < 0)
+    {
+      printf ("Could not write %s\n", script1);
+      exit (1);
+    }
+  close (fd);
+
+  const char content2[] = "echo script $1\n";
+  fd = open (script2, O_WRONLY | O_CREAT, 0700);
+  if (fd < 0
+      || TEMP_FAILURE_RETRY (write (fd, content2, sizeof content2))
+	 != sizeof content2
+      || fchmod (fd, S_IRUSR | S_IXUSR) < 0)
+    {
+      printf ("Could not write %s\n", script2);
+      exit (1);
+    }
+  close (fd);
+}

	Jakub

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] Use alloca in execvp if possible
  2007-01-03 13:15 [PATCH] Use alloca in execvp if possible Jakub Jelinek
@ 2007-01-03 23:03 ` Ulrich Drepper
  0 siblings, 0 replies; 2+ messages in thread
From: Ulrich Drepper @ 2007-01-03 23:03 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Glibc hackers

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

Applied after adding some changes to remove warnings and to avoid
depending on files in /tmp being executable.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2007-01-03 23:03 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-03 13:15 [PATCH] Use alloca in execvp if possible Jakub Jelinek
2007-01-03 23:03 ` Ulrich Drepper

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).