Re: [PATCH 02/10] pasta: More general way of starting spawned shell as a login shell

[Stefano Brivio <sbrivio@redhat.com>]

Just nits here:

On Tue, 11 Oct 2022 16:40:10 +1100
David Gibson <david@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@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