On 2023/05/18 09:11:59 -0600, Todd C. Miller <[email protected]> wrote:
> The way user(8) runs commands via system(3) is fragile. It does
> not correctly handle paths with whitespace or shell metacharacters.
> Rather than try to quote everything (which is also fragile) I think
> it is safest to just exec the commands directly.
when I saw that the previous change was to remove the only "cd xyz &&
cmd" I hoped that the follow up was to use exec instead of system :)
> OK?
verified that it works as intended. -v is actually nicer now, since
it logs the commands it executes (which is expected as per man page
but not as per previous behaviour.)
for what is worth ok op@ (with or without nits below)
> - todd
>
> Index: usr.sbin/user/user.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/user/user.c,v
> retrieving revision 1.130
> diff -u -p -u -r1.130 user.c
> --- usr.sbin/user/user.c 16 May 2023 21:28:46 -0000 1.130
> +++ usr.sbin/user/user.c 18 May 2023 15:10:52 -0000
> @@ -31,6 +31,7 @@
>
> #include <sys/types.h>
> #include <sys/stat.h>
> +#include <sys/wait.h>
>
> #include <ctype.h>
> #include <dirent.h>
> @@ -42,7 +43,7 @@
> #include <login_cap.h>
> #include <paths.h>
> #include <pwd.h>
> -#include <stdarg.h>
> +#include <stdbool.h>
existing code just used 0 and 1 (e.g. creategid.) lone zeros as
arguments are not really readable, but still I can't make myself
liking stdbool. not that my tastes matters, but it seems to be used
very sparsingly in usr.sbin
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -103,6 +104,10 @@ enum {
> F_ACCTUNLOCK = 0x10000
> };
>
> +/* bit flags for runas() */
> +#define RUNAS_DUP_DEVNULL 0x01
> +#define RUNAS_IGNORE_EXITVAL 0x02
another nit: the other flags are defined within an enums.
> [...]
> -/* a replacement for system(3) */
> +/* run the given command with optional arguments as the specified uid */
> static int
> -asystem(const char *fmt, ...)
> +runas(const char *path, const char *const argv[], uid_t uid, int flags)
> {
> [...]
> + default:
> + while (waitpid(child, &status, 0) == -1) {
> + if (errno != EINTR)
> + err(EXIT_FAILURE, "waitpid");
> + }
> + if (WIFSIGNALED(status)) {
> + ret = WTERMSIG(status);
> + warnx("[Warning] `%s' killed by signal %d", buf, ret);
> + ret |= 128;
> + } else {
> + if (!(flags & RUNAS_IGNORE_EXITVAL))
> + ret = WEXITSTATUS(status);
> + if (ret != 0)
> + warnx("[Warning] unable to run `%s'", buf);
more than "unable to run" I'd say "failed to run" or "command
failed" / "exited with nonzero status".
"unable to run" makes me think that it failed to exec the
program, rather than it failed at runtime.
> + }
> + return ret;
> }
> - return ret;
> +}
> +
> +/* run the given command with optional arguments */
> +static int
> +run(const char *path, const char *const argv[])
> +{
> + return runas(path, argv, -1, false);
> }
