From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH 02/10] pasta: More general way of starting spawned shell as a login shell
Date: Fri, 14 Oct 2022 10:24:43 +1100 [thread overview]
Message-ID: <Y0ieO6yelX+zJpin@yekko> (raw)
In-Reply-To: <20221013114818.247a7cc2@elisabeth>
[-- Attachment #1: Type: text/plain, Size: 4833 bytes --]
On Thu, Oct 13, 2022 at 11:48:18AM +0200, Stefano Brivio wrote:
> On Thu, 13 Oct 2022 19:22:27 +1100
> David Gibson <david@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@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.
> >
> > 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;
Ah, right, that makes sense. Adjusted.
> > > > 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 ;)
I see what you mean about the crampedness. But the proposed
alternative seems more complicated in other ways to me. I think I'll
leave it as is for now.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2022-10-13 23:42 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-11 5:40 [PATCH 00/10] Fixes and cleanups for capability handling David Gibson
2022-10-11 5:40 ` [PATCH 01/10] test: Move slower tests to end of test run David Gibson
2022-10-11 5:40 ` [PATCH 02/10] pasta: More general way of starting spawned shell as a login shell David Gibson
2022-10-13 2:16 ` Stefano Brivio
2022-10-13 8:22 ` David Gibson
2022-10-13 9:48 ` Stefano Brivio
2022-10-13 23:24 ` David Gibson [this message]
2022-10-11 5:40 ` [PATCH 03/10] pasta_start_ns() always ends in parent context David Gibson
2022-10-11 5:40 ` [PATCH 04/10] Remove unhelpful drop_caps() call in pasta_start_ns() David Gibson
2022-10-11 5:40 ` [PATCH 05/10] Clarify various self-isolation steps David Gibson
2022-10-13 2:17 ` Stefano Brivio
2022-10-13 8:31 ` David Gibson
2022-10-13 12:49 ` Stefano Brivio
2022-10-13 23:25 ` David Gibson
2022-10-11 5:40 ` [PATCH 06/10] Replace FWRITE with a function David Gibson
2022-10-13 2:17 ` Stefano Brivio
2022-10-13 8:51 ` David Gibson
2022-10-11 5:40 ` [PATCH 07/10] isolation: Replace drop_caps() with a version that actually does something David Gibson
2022-10-13 2:18 ` Stefano Brivio
2022-10-13 9:44 ` David Gibson
2022-10-13 4:01 ` Stefano Brivio
2022-10-13 13:08 ` Stefano Brivio
2022-10-13 16:37 ` Stefano Brivio
2022-10-13 23:42 ` David Gibson
2022-10-11 5:40 ` [PATCH 08/10] isolation: Prevent any child processes gaining capabilities David Gibson
2022-10-13 2:17 ` Stefano Brivio
2022-10-13 9:33 ` David Gibson
2022-10-13 9:50 ` Stefano Brivio
2022-10-11 5:40 ` [PATCH 09/10] isolation: Only configure UID/GID mappings in userns when spawning shell David Gibson
2022-10-13 2:18 ` Stefano Brivio
2022-10-13 9:36 ` David Gibson
2022-10-11 5:40 ` [PATCH 10/10] Rename pasta_setup_ns() to pasta_spawn_cmd() David Gibson
2022-10-13 2:44 ` [PATCH 00/10] Fixes and cleanups for capability handling Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y0ieO6yelX+zJpin@yekko \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).