Just nits here: On Tue, 11 Oct 2022 16:40:10 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:When invoked so as to spawn a shell, pasta checks explicitly for the shell being bash and if so, adds a "-l" option to make it a login shell. This is not ideal, since this is a bash specific option and requires pasta to know about specific shell variants. There's a general convention for starting a login shell, which is to prepend a "-" to argv[0]. Use this approach instead, so we don't need bash specific logic.Hah, I didn't know that was the meaning.Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- pasta.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/pasta.c b/pasta.c index 1dd8267..7c3acef 100644 --- a/pasta.c +++ b/pasta.c @@ -148,10 +148,12 @@ void pasta_open_ns(struct ctx *c, const char *netns) /** * struct pasta_setup_ns_arg - Argument for pasta_setup_ns() + * @exe: Executable to run * @argv: Command and arguments to run */ struct pasta_setup_ns_arg { - char **argv; + const char *exe; + char *const *argv; }; /** @@ -162,12 +164,13 @@ struct pasta_setup_ns_arg { */ static int pasta_setup_ns(void *arg) { - struct pasta_setup_ns_arg *a = (struct pasta_setup_ns_arg *)arg; + const struct pasta_setup_ns_arg *a + = (const struct pasta_setup_ns_arg *)arg;At this point the assignment could be split onto another line.FWRITE("/proc/sys/net/ipv4/ping_group_range", "0 0", "Cannot set ping_group_range, ICMP requests might fail"); - execvp(a->argv[0], a->argv); + execvp(a->exe, a->argv); perror("execvp"); exit(EXIT_FAILURE); @@ -182,26 +185,31 @@ static int pasta_setup_ns(void *arg) void pasta_start_ns(struct ctx *c, int argc, char *argv[]) { struct pasta_setup_ns_arg arg = { + .exe = argv[0], .argv = argv, }; - char *shell = getenv("SHELL"); - char *sh_argv[] = { shell, NULL }; - char *bash_argv[] = { shell, "-l", NULL }; + char *sh_argv[] = { NULL, NULL }; char ns_fn_stack[NS_FN_STACK_SIZE];If you respin, it would be nice to have the usual ordering here (sh_argv[] after ns_fn_stack).+ char sh_arg0[PATH_MAX + 1]; c->foreground = 1; if (!c->debug) c->quiet = 1; - if (!shell) - shell = "/bin/sh"; if (argc == 0) { - if (strstr(shell, "/bash")) { - arg.argv = bash_argv; - } else { - arg.argv = sh_argv; + arg.exe = getenv("SHELL"); + if (!arg.exe) + arg.exe = "/bin/sh"; + + if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0), + "-%s", arg.exe) >= sizeof(sh_arg0)) {This is completely specified and looks safe, but it also looks more complicated than it actually is, at a glance. Maybe a separate length check before snprintf() would make it look more natural. Not a strong preference though.+ err("$SHELL is too long (%u bytes)", + strlen(arg.exe)); + exit(EXIT_FAILURE); } + sh_argv[0] = sh_arg0; + arg.argv = sh_argv; } pasta_child_pid = clone(pasta_setup_ns,-- Stefano