Message ID | b45e848c-b47c-fa9e-6873-7b191efb982e@linaro.org |
---|---|
State | Dropped |
Headers |
Received: (qmail 13900 invoked by alias); 22 Nov 2016 13:28:29 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 13888 invoked by uid 89); 22 Nov 2016 13:28:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=no X-HELO: mail-vk0-f41.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=uK0VDuvMqjOPwAkKdRnsWH1qMjELQaUulMYJElrXI3M=; b=KY6x2emqvYiFIPv0gfVZVeuYJW7kubkHWtUFwgXY6t6ZDbuTnznugHqvJF3j0fR3W6 2p/gFGelZyKyRigiSZatZ7jv4i4jSboOIrbuUq3A6O+IffbkEzB6zhJgNzXI75AzlbTP QcZ/p61Y3xHgVS6BD7oHdJvhOD1tI0mksH2ZgKyAIS1UlZ2yH8170wy8x7CpuZTUJP3k /ofCSWSYCQ/K++h4c6Is0PhJEbulIrdD+rH0g1YAS2nwU4GNwkQ/dX9mH072tzQuuNBu jltoIVcWNii4xCdh4wjAYWXpyURtwZT3E1fWKnrUWDcMl4j1n1RI3zUKA9BDWWNFifUw S73g== X-Gm-Message-State: AKaTC00qW8YqDIzJDk4x3mKrVI7B+A465Z8Qd+bSYeNXZHmqzSXp2DnkRMVqcH+wSUFI6+Vs X-Received: by 10.31.214.65 with SMTP id n62mr9539446vkg.127.1479821297113; Tue, 22 Nov 2016 05:28:17 -0800 (PST) Subject: Re: [PATCH v2] Fix writes past the allocated array bounds in execvpe (BZ#20847) To: libc-alpha@sourceware.org, Andreas Schwab <schwab@linux-m68k.org> References: <1479755377-12442-1-git-send-email-adhemerval.zanella@linaro.org> <87vavg7fx1.fsf@linux-m68k.org> <ba4c0f7f-35af-a733-71c1-c035b3c3a495@linaro.org> <87r3647e8x.fsf@linux-m68k.org> <20161122101759.GA30936@linux.vnet.ibm.com> From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Message-ID: <b45e848c-b47c-fa9e-6873-7b191efb982e@linaro.org> Date: Tue, 22 Nov 2016 11:28:12 -0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161122101759.GA30936@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit |
Commit Message
Adhemerval Zanella Netto
Nov. 22, 2016, 1:28 p.m. UTC
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.
Comments
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
On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote: > On 11/22/2016 02:28 PM, Adhemerval Zanella wrote: > >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; > > > > > 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. Sigh, seems to be virtually impossible to get this right. > 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. Stefan and me tried to understand the Posix spec for the exec function family, and it's not exactly clear to us whether a NULL argument list is a usage error or not. However, since it's possible to support that case, and funny applications may rely on it, so __execvpe should probably accept that case. Maybe fix it with char *new_argv[(argc > 0) ? 2 + argc: 3]; ? Ciao Dominik ^_^ ^_^
On 24/11/2016 09:10, Dominik Vogt wrote: > On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote: >> On 11/22/2016 02:28 PM, Adhemerval Zanella wrote: >>> 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; >>> >>> > >> 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. > > Sigh, seems to be virtually impossible to get this right. > >> 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. > > Stefan and me tried to understand the Posix spec for the exec > function family, and it's not exactly clear to us whether a NULL > argument list is a usage error or not. However, since it's > possible to support that case, and funny applications may rely on > it, so __execvpe should probably accept that case. > > Maybe fix it with > > char *new_argv[(argc > 0) ? 2 + argc: 3]; > > ? This is pretty much what I have proposed in my last patch iteration [1] (I used your suggestion on accounting the arguments). POSIX is indeed not specific, but since old execvp{e} implementation used to support it, I think we should keep supporting it due compatibility. [1] https://sourceware.org/ml/libc-alpha/2016-11/msg00832.html
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;