Re: Possible bug in lib/libc/gen/exec.c
Hello Theo,
On 9/16/21 10:53 PM, Theo de Raadt wrote:
@@ -45,25 +46,31 @@ execl(const char *name, const char *arg,
{
va_list ap;
char **argv;
- int n;
+ size_t maplen;
+ int save_errno, n, error;
See below.
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
- argv = alloca((n + 1) * sizeof(*argv));
- if (argv == NULL) {
- errno = ENOMEM;
+ maplen = (n + 1) * sizeof(*argv);
+ argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (argv == MAP_FAILED)
return (-1);
- }
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
while ((argv[n] = va_arg(ap, char *)) != NULL)
n++;
va_end(ap);
- return (execve(name, argv, environ));
+
+ error = execve(name, argv, environ);
+ save_errno = errno;
+ munmap(argv, maplen);
+ errno = save_errno;
+ return (error);
This part could be simplified to:
(void) execve(name, argv, environ);
save_errno = errno;
munmap(argv, maplen);
errno = save_errno;
return -1;
Allowing to remove the error variable. execve(2) always returns -1.
}
DEF_WEAK(execl);
@@ -72,18 +79,21 @@ execle(const char *name, const char *arg
{
va_list ap;
char **argv, **envp;
- int n;
+ size_t maplen;
+ int save_errno, n, error;
Same here.
@@ -91,7 +101,12 @@ execle(const char *name, const char *arg
n++;
envp = va_arg(ap, char **);
va_end(ap);
- return (execve(name, argv, envp));
+
+ error = execve(name, argv, envp);
+ save_errno = errno;
+ munmap(argv, maplen);
+ errno = save_errno;
+ return error;
Same here
@@ -99,25 +114,32 @@ execlp(const char *name, const char *arg
{
va_list ap;
char **argv;
- int n;
+ size_t maplen;
+ int save_errno, n, error;
Same here.
--
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/
Re: Possible bug in lib/libc/gen/exec.c
we had the same issue in bionic when we removed all our alloca()s, modulo
the fact that ours is a VLA rather than alloca(), but same thing:
https://android.googlesource.com/platform/bionic/+/master/libc/bionic/exec.cpp#61
we argued that it doesn't matter in this case though because we'll touch
all the entries, in order, so there's no possibility of jumping off the end
of the stack onto another VMA.
(we have used mmap() instead in more complicated cases though.)
On Thu, Sep 16, 2021 at 1:39 PM Theo de Raadt wrote:
> Alejandro Colomar (man-pages) wrote:
>
> > Hi,
> >
> > I don't know if OpenBSD has a different implementation of alloca(3)
> > than Linux. In Linux, alloca(3) (a.k.a. __builtin_alloca()) can't
> > return NULL, as it can't detect errors.
>
> There are no alloca can return NULL.
>
> > The only way to detect an
> > error is to add a handler for SIGSEGV, AFAIK.
>
> Actually, it is worse than that. Massive alloca have been encountered
> which reach off the stack. For over 20 years we've been on a mission
> to delete alloca in our tree, and these are the last ones that remain,
> because other allocators are very difficult to use in this place.
>
> Maybe we should investigate using mmap. Of the 4 cases, 3 are not
> too difficult, but the 4th case will be very messy, including unwind
> for the 3rd case.
>
> > I found the following code in :
> >
> > argv = alloca((n + 1) * sizeof(*argv));
> > if (argv == NULL) {
> > errno = ENOMEM;
> > return (-1);
> > }
> >
> > But of course, that NULL should never happen, right?
>
> > As a side note, I think you could encapsulate the code calling
> > alloca(3) into a helper macro, to avoid repetition.
>
> I don't see how that would help anything.
>
> > I can prepare a patch if you confirm the bug.
>
> What do you propose to do in your patch?
>
>
>
Re: Possible bug in lib/libc/gen/exec.c
It always returns -1 until the world changes in some subtle way,
then the code is wrong.
The logic is supposed to return what execve returns, not reinvent
the value.
Over decades this kind of assumption can turn into a bug, so I
prefer to do it right.
Alejandro Colomar (man-pages) wrote:
> Hello Theo,
>
> On 9/16/21 10:53 PM, Theo de Raadt wrote:
> > @@ -45,25 +46,31 @@ execl(const char *name, const char *arg,
> > {
> > va_list ap;
> > char **argv;
> > - int n;
> > + size_t maplen;
> > + int save_errno, n, error;
>
> See below.
>
> > va_start(ap, arg);
> > n = 1;
> > while (va_arg(ap, char *) != NULL)
> > n++;
> > va_end(ap);
> > - argv = alloca((n + 1) * sizeof(*argv));
> > - if (argv == NULL) {
> > - errno = ENOMEM;
> > + maplen = (n + 1) * sizeof(*argv);
> > + argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
> > + MAP_ANON|MAP_PRIVATE, -1, 0);
> > + if (argv == MAP_FAILED)
> > return (-1);
> > - }
> > va_start(ap, arg);
> > n = 1;
> > argv[0] = (char *)arg;
> > while ((argv[n] = va_arg(ap, char *)) != NULL)
> > n++;
> > va_end(ap);
> > - return (execve(name, argv, environ));
> > +
> > + error = execve(name, argv, environ);
> > + save_errno = errno;
> > + munmap(argv, maplen);
> > + errno = save_errno;
> > + return (error);
>
> This part could be simplified to:
>
> (void) execve(name, argv, environ);
> save_errno = errno;
> munmap(argv, maplen);
> errno = save_errno;
> return -1;
>
> Allowing to remove the error variable. execve(2) always returns -1.
>
> > }
> > DEF_WEAK(execl);
> > @@ -72,18 +79,21 @@ execle(const char *name, const char *arg
> > {
> > va_list ap;
> > char **argv, **envp;
> > - int n;
> > + size_t maplen;
> > + int save_errno, n, error;
>
> Same here.
>
> > @@ -91,7 +101,12 @@ execle(const char *name, const char *arg
> > n++;
> > envp = va_arg(ap, char **);
> > va_end(ap);
> > - return (execve(name, argv, envp));
> > +
> > + error = execve(name, argv, envp);
> > + save_errno = errno;
> > + munmap(argv, maplen);
> > + errno = save_errno;
> > + return error;
>
> Same here
>
> > @@ -99,25 +114,32 @@ execlp(const char *name, const char *arg
> > {
> > va_list ap;
> > char **argv;
> > - int n;
> > + size_t maplen;
> > + int save_errno, n, error;
>
> Same here.
>
>
>
> --
> Alejandro Colomar
> Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
> http://www.alejandro-colomar.es/
Re: Possible bug in lib/libc/gen/exec.c
enh wrote: > we had the same issue in bionic when we removed all our alloca()s, modulo the > fact > that ours is a VLA rather than alloca(), but same thing: > https://android.googlesource.com/platform/bionic/+/master/libc/bionic/exec.cpp#61 that cargo culting doesn't fix anything... > we argued that it doesn't matter in this case though because we'll touch all > the > entries, in order, so there's no possibility of jumping off the end of the > stack onto > another VMA. we've always had a sufficient gap beyond the end of our stack, but a few years ago many people freaked out because they didn't. nowadays, that is enforced on a system level, so that door is largely closed, dependent on linear access. > (we have used mmap() instead in more complicated cases though.) yes, that is the solution, but rollback and errno must be handled carefully. If this mmap() fix goes in, the only alloca() remain in our tree are: clang - seems to be in test code only binutils - shocking 1 in gcc configure 1 in perl - non-openbsd cases 1 small one in libcrypto - someone should go fix that
Re: Possible bug in lib/libc/gen/exec.c
Theo de Raadt wrote:
> Maybe we should investigate using mmap. Of the 4 cases, 3 are not
> too difficult, but the 4th case will be very messy, including unwind
> for the 3rd case.
Here is a version that uses mmap instead of alloca, including rollback
of resource allocations in case of failure. This is similar to the mmap
use in vfprintf which I added many years ago to support positional
arguments without alloca or malloc, so that snprintf family can be
remain maximally reentrant. So I've been here before...
I have done a little testing, but it may still have bugs.
Index: lib/libc/gen/exec.c
===
RCS file: /cvs/src/lib/libc/gen/exec.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 exec.c
--- lib/libc/gen/exec.c 13 Mar 2016 18:34:20 - 1.23
+++ lib/libc/gen/exec.c 16 Sep 2021 20:48:25 -
@@ -30,6 +30,7 @@
#include
#include
+#include
#include
#include
@@ -45,25 +46,31 @@ execl(const char *name, const char *arg,
{
va_list ap;
char **argv;
- int n;
+ size_t maplen;
+ int save_errno, n, error;
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
- argv = alloca((n + 1) * sizeof(*argv));
- if (argv == NULL) {
- errno = ENOMEM;
+ maplen = (n + 1) * sizeof(*argv);
+ argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (argv == MAP_FAILED)
return (-1);
- }
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
while ((argv[n] = va_arg(ap, char *)) != NULL)
n++;
va_end(ap);
- return (execve(name, argv, environ));
+
+ error = execve(name, argv, environ);
+ save_errno = errno;
+ munmap(argv, maplen);
+ errno = save_errno;
+ return (error);
}
DEF_WEAK(execl);
@@ -72,18 +79,21 @@ execle(const char *name, const char *arg
{
va_list ap;
char **argv, **envp;
- int n;
+ size_t maplen;
+ int save_errno, n, error;
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
- argv = alloca((n + 1) * sizeof(*argv));
- if (argv == NULL) {
- errno = ENOMEM;
+
+ maplen = (n + 1) * sizeof(*argv);
+ argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (argv == MAP_FAILED)
return (-1);
- }
+
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
@@ -91,7 +101,12 @@ execle(const char *name, const char *arg
n++;
envp = va_arg(ap, char **);
va_end(ap);
- return (execve(name, argv, envp));
+
+ error = execve(name, argv, envp);
+ save_errno = errno;
+ munmap(argv, maplen);
+ errno = save_errno;
+ return error;
}
int
@@ -99,25 +114,32 @@ execlp(const char *name, const char *arg
{
va_list ap;
char **argv;
- int n;
+ size_t maplen;
+ int save_errno, n, error;
va_start(ap, arg);
n = 1;
while (va_arg(ap, char *) != NULL)
n++;
va_end(ap);
- argv = alloca((n + 1) * sizeof(*argv));
- if (argv == NULL) {
- errno = ENOMEM;
+
+ maplen = (n + 1) * sizeof(*argv);
+ argv = mmap(NULL, maplen, PROT_WRITE|PROT_READ,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (argv == MAP_FAILED)
return (-1);
- }
+
va_start(ap, arg);
n = 1;
argv[0] = (char *)arg;
while ((argv[n] = va_arg(ap, char *)) != NULL)
n++;
va_end(ap);
- return (execvp(name, argv));
+ error = execvp(name, argv);
+ save_errno = errno;
+ munmap(argv, maplen);
+ errno = save_errno;
+ return error;
}
int
@@ -132,10 +154,12 @@ execvpe(const char *name, char *const *a
{
char **memp;
int cnt;
- size_t lp, ln, len;
+ size_t lp, ln, curlen;
char *p;
int eacces = 0;
char *bp, *cur, *path, buf[PATH_MAX];
+ size_t maplen;
+ int save_errno, n;
/*
* Do not allow null name
@@ -156,13 +180,14 @@ execvpe(const char *name, char *const *a
/* Get the path we're searching. */
if (!(path = getenv("PATH")))
path = _PATH_DEFPATH;
- len = strlen(path) + 1;
- cur = alloca(len);
- if (cur == NULL) {
- errno = ENOMEM;
+
+ curlen = strlen(path) + 1;
+ cur = mmap(NULL, curlen, PROT_WRITE|PROT_READ,
+ MAP_ANON|MAP_PRIVATE, -1, 0);
+ if (cur == MAP_FAILED)
return (-1);
- }
- strlcpy(cur, path, len);
+
+ strlcpy(cur, path, curlen);
path = cur;
while ((p = strsep(, ":"))) {
Re: Possible bug in lib/libc/gen/exec.c
Alejandro Colomar (man-pages) wrote:
> Hi,
>
> I don't know if OpenBSD has a different implementation of alloca(3)
> than Linux. In Linux, alloca(3) (a.k.a. __builtin_alloca()) can't
> return NULL, as it can't detect errors.
There are no alloca can return NULL.
> The only way to detect an
> error is to add a handler for SIGSEGV, AFAIK.
Actually, it is worse than that. Massive alloca have been encountered
which reach off the stack. For over 20 years we've been on a mission
to delete alloca in our tree, and these are the last ones that remain,
because other allocators are very difficult to use in this place.
Maybe we should investigate using mmap. Of the 4 cases, 3 are not
too difficult, but the 4th case will be very messy, including unwind
for the 3rd case.
> I found the following code in :
>
> argv = alloca((n + 1) * sizeof(*argv));
> if (argv == NULL) {
> errno = ENOMEM;
> return (-1);
> }
>
> But of course, that NULL should never happen, right?
> As a side note, I think you could encapsulate the code calling
> alloca(3) into a helper macro, to avoid repetition.
I don't see how that would help anything.
> I can prepare a patch if you confirm the bug.
What do you propose to do in your patch?
