Changeset View
Standalone View
lib/libc/gen/exec.c
Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines | |||||
#include <paths.h> | #include <paths.h> | ||||
#include <stdarg.h> | #include <stdarg.h> | ||||
#include "un-namespace.h" | #include "un-namespace.h" | ||||
#include "libc_private.h" | #include "libc_private.h" | ||||
extern char **environ; | extern char **environ; | ||||
static const char execvPe_err_preamble[] = "execvP: "; | |||||
static const char execvPe_err_trailer[] = ": path too long\n"; | |||||
int | int | ||||
execl(const char *name, const char *arg, ...) | execl(const char *name, const char *arg, ...) | ||||
{ | { | ||||
va_list ap; | va_list ap; | ||||
const char **argv; | const char **argv; | ||||
int n; | int n; | ||||
va_start(ap, arg); | va_start(ap, arg); | ||||
▲ Show 20 Lines • Show All 84 Lines • ▼ Show 20 Lines | |||||
static int | static int | ||||
execvPe(const char *name, const char *path, char * const *argv, | execvPe(const char *name, const char *path, char * const *argv, | ||||
char * const *envp) | char * const *envp) | ||||
{ | { | ||||
const char **memp; | const char **memp; | ||||
size_t cnt, lp, ln; | size_t cnt, lp, ln; | ||||
int eacces, save_errno; | int eacces, save_errno; | ||||
char *cur, buf[MAXPATHLEN]; | char buf[MAXPATHLEN]; | ||||
const char *p, *bp; | const char *bp, *np, *op, *p; | ||||
struct stat sb; | struct stat sb; | ||||
eacces = 0; | eacces = 0; | ||||
/* If it's an absolute or relative path name, it's easy. */ | /* If it's an absolute or relative path name, it's easy. */ | ||||
if (strchr(name, '/')) { | if (strchr(name, '/')) { | ||||
bp = name; | bp = name; | ||||
cur = NULL; | |||||
goto retry; | goto retry; | ||||
jilles: There needs to be `op = NULL;` here so that the path search loop is not entered with… | |||||
Done Inline ActionsGood catch, will fix kevans: Good catch, will fix | |||||
} | } | ||||
bp = buf; | bp = buf; | ||||
/* If it's an empty path name, fail in the usual POSIX way. */ | /* If it's an empty path name, fail in the usual POSIX way. */ | ||||
if (*name == '\0') { | if (*name == '\0') { | ||||
errno = ENOENT; | errno = ENOENT; | ||||
return (-1); | return (-1); | ||||
} | } | ||||
cur = alloca(strlen(path) + 1); | op = path; | ||||
if (cur == NULL) { | ln = strlen(name); | ||||
errno = ENOMEM; | while (op != NULL) { | ||||
return (-1); | np = strchrnul(op, ':'); | ||||
} | |||||
strcpy(cur, path); | |||||
while ((p = strsep(&cur, ":")) != NULL) { | |||||
/* | /* | ||||
* It's a SHELL path -- double, leading and trailing colons | * It's a SHELL path -- double, leading and trailing colons | ||||
* mean the current directory. | * mean the current directory. | ||||
*/ | */ | ||||
if (*p == '\0') { | if (*np == '\0') { | ||||
andrew_tao173.riddles.org.ukUnsubmitted Done Inline ActionsOn second look this logic looks like it could be a lot simpler; as far as I can tell the two blocks for leading/trailing/double colon can be merged, and so can the ones for final and non-final components: something like, if (np == op) { /* empty component: */ p = "."; lp = 1; } else { /* non-empty component: */ p = op; lp = np - op; } or am I missing something? andrew_tao173.riddles.org.uk: On second look this logic looks like it could be a lot simpler; as far as I can tell the two… | |||||
kevansAuthorUnsubmitted Done Inline ActionsWhoops- looks like I forgot to re-evaluate and simplify after strchr -> strchrnul kevans: Whoops- looks like I forgot to re-evaluate and simplify after `strchr` -> `strchrnul` | |||||
if (op == np) { | |||||
/* Trailing colon. */ | |||||
p = "."; | p = "."; | ||||
lp = 1; | lp = 1; | ||||
} else | } else { | ||||
/* Final non-empty component. */ | |||||
p = op; | |||||
lp = strlen(p); | lp = strlen(p); | ||||
ln = strlen(name); | } | ||||
} else if (np == op) { | |||||
/* Double colon in the middle, or a leading colon. */ | |||||
p = "."; | |||||
lp = 1; | |||||
} else { | |||||
/* Standard non-empty component. */ | |||||
p = op; | |||||
lp = np - op; | |||||
} | |||||
/* | /* | ||||
* If the path is too long complain. This is a possible | * If the path is too long complain. This is a possible | ||||
* security issue; given a way to make the path too long | * security issue; given a way to make the path too long | ||||
* the user may execute the wrong program. | * the user may execute the wrong program. | ||||
*/ | */ | ||||
if (lp + ln + 2 > sizeof(buf)) { | if (lp + ln + 2 > sizeof(buf)) { | ||||
(void)_write(STDERR_FILENO, "execvP: ", 8); | (void)_write(STDERR_FILENO, execvPe_err_preamble, | ||||
sizeof(execvPe_err_preamble) - 1); | |||||
(void)_write(STDERR_FILENO, p, lp); | (void)_write(STDERR_FILENO, p, lp); | ||||
(void)_write(STDERR_FILENO, ": path too long\n", | (void)_write(STDERR_FILENO, execvPe_err_trailer, | ||||
16); | sizeof(execvPe_err_trailer) - 1); | ||||
Done Inline ActionsI think this line (and 206) can be improved by moving the string to const array and using sizeof(). kib: I think this line (and 206) can be improved by moving the string to const array and using… | |||||
continue; | continue; | ||||
Not Done Inline ActionsThis "continue" is wrong now that the advancement of the loop pointer has been moved to below it - it'll loop forever andrew_tao173.riddles.org.uk: This "continue" is wrong now that the advancement of the loop pointer has been moved to below… | |||||
} | } | ||||
bcopy(p, buf, lp); | bcopy(p, buf, lp); | ||||
buf[lp] = '/'; | buf[lp] = '/'; | ||||
bcopy(name, buf + lp + 1, ln); | bcopy(name, buf + lp + 1, ln); | ||||
buf[lp + ln + 1] = '\0'; | buf[lp + ln + 1] = '\0'; | ||||
/* Advance */ | |||||
if (*np == '\0') | |||||
op = NULL; | |||||
else | |||||
op = np + 1; | |||||
retry: (void)_execve(bp, argv, envp); | retry: (void)_execve(bp, argv, envp); | ||||
switch (errno) { | switch (errno) { | ||||
case E2BIG: | case E2BIG: | ||||
goto done; | goto done; | ||||
case ELOOP: | case ELOOP: | ||||
case ENAMETOOLONG: | case ENAMETOOLONG: | ||||
case ENOENT: | case ENOENT: | ||||
break; | break; | ||||
case ENOEXEC: | case ENOEXEC: | ||||
for (cnt = 0; argv[cnt]; ++cnt) | for (cnt = 0; argv[cnt]; ++cnt) | ||||
; | ; | ||||
memp = alloca((cnt + 2) * sizeof(char *)); | memp = alloca((cnt + 2) * sizeof(char *)); | ||||
Done Inline ActionsThis is an alloca() as well, for which the stack allocated by do_posix_spawn() may not be sufficient. Perhaps do_posix_spawnp() should reserve enough space for this allocation if it should happen. jilles: This is an `alloca()` as well, for which the stack allocated by `do_posix_spawn()` may not be… | |||||
Done Inline ActionsThere is a follow-up update that did include this. =) kevans: There is a follow-up update that did include this. =) | |||||
if (memp == NULL) { | if (memp == NULL) { | ||||
/* errno = ENOMEM; XXX override ENOEXEC? */ | /* errno = ENOMEM; XXX override ENOEXEC? */ | ||||
goto done; | goto done; | ||||
} | } | ||||
memp[0] = "sh"; | memp[0] = "sh"; | ||||
memp[1] = bp; | memp[1] = bp; | ||||
bcopy(argv + 1, memp + 2, cnt * sizeof(char *)); | bcopy(argv + 1, memp + 2, cnt * sizeof(char *)); | ||||
Not Done Inline ActionsThere's another bug here, though I don't think it's a security issue; cnt can be 0, which means that the memp array doesn't get terminated, and execve runs off the end of it and passes garbage args to the invoked script. Maybe add 3 rather than 2 to cnt above, and have: if (cnt > 0) bcopy(argv + 1, memp + 2, cnt * sizeof(char*)); else memp[2] = NULL; ` andrew_tao173.riddles.org.uk: There's another bug here, though I don't think it's a security issue; `cnt` can be 0, which… | |||||
(void)_execve(_PATH_BSHELL, | (void)_execve(_PATH_BSHELL, | ||||
__DECONST(char **, memp), envp); | __DECONST(char **, memp), envp); | ||||
goto done; | goto done; | ||||
case ENOMEM: | case ENOMEM: | ||||
goto done; | goto done; | ||||
case ENOTDIR: | case ENOTDIR: | ||||
break; | break; | ||||
case ETXTBSY: | case ETXTBSY: | ||||
▲ Show 20 Lines • Show All 48 Lines • Show Last 20 Lines |
There needs to be op = NULL; here so that the path search loop is not entered with uninitialized op if the _execve() fails with ENOENT or a similar error.