This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] Fix writes past the allocated array bounds in execvpe (BZ#20847)


On 11/22/2016 02:28 PM, Adhemerval Zanella wrote:


On 22/11/2016 08:17, Dominik Vogt wrote:
On Mon, Nov 21, 2016 at 09:46:22PM +0100, Andreas Schwab wrote:
On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

With this change are you ok to push this in?

Yes, this is ok.

No!  The patch writes past the array bounds in the else branch.

Ciao

Dominik ^_^  ^_^


Since I made a mistake to push this patch, which I apologize, I think
your previous suggestions is indeed the correct one (patch below).
Reading again, it indeed seems simpler to just account for arguments
and not the final 'NULL' since the 'argv + 1' will indeed ignore
the script name.

diff --git a/posix/execvpe.c b/posix/execvpe.c
index 7cdb06a..cf167d0 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -38,8 +38,8 @@
 static void
 maybe_script_execute (const char *file, char *const argv[], char *const envp[])
 {
-  ptrdiff_t argc = 0;
-  while (argv[argc++] != NULL)
+  ptrdiff_t argc;
+  for (argc = 0; argv[argc] != NULL; argc++)
     {
       if (argc == INT_MAX - 1)
        {
@@ -50,11 +50,11 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])

   /* Construct an argument list for the shell.  It will contain at minimum 3
      arguments (current shell, script, and an ending NULL.  */
-  char *new_argv[argc + 1];
+  char *new_argv[2 + argc];
   new_argv[0] = (char *) _PATH_BSHELL;
   new_argv[1] = (char *) file;
   if (argc > 1)
-    memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
+    memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
   else
     new_argv[2] = NULL;



Hi Adhemerval,

there is an additional special case.
If someone passes an argv array with argv[0] = NULL, then argc is zero and new_argv contains two elements but the else path writes NULL to new_argv[2], which is past the allocated array bounds.

I don't know if this case is allowed at all.
According to the man-page:
The first argument, by convention, should point to the
filename associated with the file being executed.

Bye
Stefan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]