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

On Thu, 13 Oct 2022 19:22:27 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Thu, Oct 13, 2022 at 04:16:59AM +0200, Stefano
Brivio wrote:
  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.   
 
 Uh.. I'm not sure what you mean by that. 
const struct pasta_setup_ns_arg *a;

a = (const struct pasta_setup_ns_arg *)arg;

...
  
 
   
  	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).   
 
 Done.
 
 
  +	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.   
 
 Uh.. not sure what you mean by that either. 
Ah, sorry.

if ((len = strlen(arg.exe)) + 1 /* - */ + 1 /* \0 */ > sizeof(sh_arg0))
	err("$SHELL is too long (%u bytes)", strlen(arg.exe));

(void)snprintf(sh_arg0, sizeof(sh_arg0), "-%s", arg.exe);

I don't know, it doesn't look so... cramped. Maybe it's just me. If it
doesn't make sense just disregard ;)

-- 
Stefano