[PATCH v2] conf: Stop parsing options at first non-option argument

[PATCH v2] conf: Stop parsing options at first non-option argument

[Stefano Brivio]

Given that pasta supports specifying a command to be executed on the
command line, even without the usual -- separator as long as there's
no ambiguity, we shouldn't eat up options that are not meant for us.

Paul reports, for instance, that with:

  pasta --config-net ip -6 route

-6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'.
That's because getopt_long(), by default, shuffles the argument list
to shift non-option arguments at the end.

Avoid that by adding '-' at the beginning of 'optstring', and mark the
position of the first non-option argument (getopt_long() will now
return the character code 1 once we hit it), so that we can use that
as command to run, or as PID for the target namespace.

Reported-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2: Instead of overriding 'name' in the getopt_long() loop, to force
    exiting the loop, adjust the exit condition

 conf.c | 23 +++++++++++++++--------
 util.c |  2 +-
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/conf.c b/conf.c
index be5259e..1a30b47 100644
--- a/conf.c
+++ b/conf.c
@@ -1240,22 +1240,22 @@ void conf(struct ctx *c, int argc, char **argv)
 	enum fwd_ports_mode fwd_default = FWD_NONE;
 	bool v4_only = false, v6_only = false;
 	struct fqdn *dnss = c->dns_search;
+	int name, ret, first_nonopt = -1;
 	unsigned int ifi4 = 0, ifi6 = 0;
 	const char *logfile = NULL;
 	const char *optstring;
 	size_t logsize = 0;
 	char *runas = NULL;
 	long fd_tap_opt;
-	int name, ret;
 	uid_t uid;
 	gid_t gid;
 
 	if (c->mode == MODE_PASTA) {
 		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
 		fwd_default = FWD_AUTO;
-		optstring = "dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
+		optstring = "-dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
 	} else {
-		optstring = "dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
+		optstring = "-dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
 	}
 
 	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
@@ -1269,6 +1269,9 @@ void conf(struct ctx *c, int argc, char **argv)
 		case -1:
 		case 0:
 			break;
+		case 1:
+			first_nonopt = optind - 1;
+			break;
 		case 2:
 			if (c->mode != MODE_PASTA)
 				die("--userns is for pasta mode only");
@@ -1629,7 +1632,10 @@ void conf(struct ctx *c, int argc, char **argv)
 			usage(argv[0], stderr, EXIT_FAILURE);
 			break;
 		}
-	} while (name != -1);
+	} while (name != -1 && name != 1);
+
+	if (first_nonopt == -1)
+		first_nonopt = optind;
 
 	if (c->mode == MODE_PASTA && !c->pasta_conf_ns) {
 		if (copy_routes_opt)
@@ -1689,9 +1695,9 @@ void conf(struct ctx *c, int argc, char **argv)
 	} while (name != -1);
 
 	if (c->mode == MODE_PASTA)
-		conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv);
-	else if (optind != argc)
-		die("Extra non-option argument: %s", argv[optind]);
+		conf_pasta_ns(&netns_only, userns, netns, first_nonopt, argc, argv);
+	else if (first_nonopt != argc)
+		die("Extra non-option argument: %s", argv[first_nonopt]);
 
 	conf_open_files(c);	/* Before any possible setuid() / setgid() */
 
@@ -1705,7 +1711,8 @@ void conf(struct ctx *c, int argc, char **argv)
 			pasta_open_ns(c, netns);
 		} else {
 			pasta_start_ns(c, uid, gid,
-				       argc - optind, argv + optind);
+				       argc - first_nonopt,
+				       argv + first_nonopt);
 		}
 	}
 
diff --git a/util.c b/util.c
index 9c6be6a..13524e6 100644
--- a/util.c
+++ b/util.c
@@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv)
 	int name;
 
 	do {
-		name = getopt_long(argc, argv, ":F", optfd, NULL);
+		name = getopt_long(argc, argv, "-:F", optfd, NULL);
 
 		if (name == 'F') {
 			errno = 0;
-- 
@@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv)
 	int name;
 
 	do {
-		name = getopt_long(argc, argv, ":F", optfd, NULL);
+		name = getopt_long(argc, argv, "-:F", optfd, NULL);
 
 		if (name == 'F') {
 			errno = 0;
-- 
2.43.0

Re: [PATCH v2] conf: Stop parsing options at first non-option argument

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

On Wed, Aug 07, 2024 at 01:28:40PM +0200, Stefano Brivio wrote:
> Given that pasta supports specifying a command to be executed on the
> command line, even without the usual -- separator as long as there's
> no ambiguity, we shouldn't eat up options that are not meant for us.
> 
> Paul reports, for instance, that with:
> 
>   pasta --config-net ip -6 route
> 
> -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'.
> That's because getopt_long(), by default, shuffles the argument list
> to shift non-option arguments at the end.
> 
> Avoid that by adding '-' at the beginning of 'optstring', and mark the
> position of the first non-option argument (getopt_long() will now
> return the character code 1 once we hit it), so that we can use that
> as command to run, or as PID for the target namespace.
> 
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Eh... I'm kind of ambivalent about the idea.  I tend to think that
accepting options in any position is generally expected behaviour, and
anything programmatically adding commands to pasta should routinely
insert a "--" (that's certainly what I do in testing code).

But, that's not a particularly strong opinion, so whatever.

The implementation looks more complex than necessary, though.  AFAICT
if you just add a '+' to the front of the optstring it will do exactly
what you want without having to juggle the first_nonopt and other
variables.

Quoting getopt_long(3)

| By default, getopt() permutes the contents of argv as it scans, so
| that eventually all the nonoptions are at the end.  Two other
| scanning modes are also implemented.  If the first character of
| optstring is '+' or the environment variable POSIXLY_CORRECT is set,
| then option processing stops as soon as a nonoption argument is
| encountered.  If '+' is not the first character of opt‐ string, it
| is treated as a normal option.  If POSIXLY_CORRECT behaviour is
| required in this case optstring will contain two '+' symbols.  If
| the first character of optstring is '-', then each nonoption
| argv-element is handled as if it were the argument of an option with
| character code 1.  (This is used by programs that were written to
| expect options and other argv-elements in any order and that care
| about the ordering of the two.)  The special argument "--" forces an
| end of option-scanning regardless of the scanning mode.

-- 
David Gibson (he or they)	| 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 --]

Re: [PATCH v2] conf: Stop parsing options at first non-option argument

[Stefano Brivio]

On Thu, 8 Aug 2024 10:57:24 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Aug 07, 2024 at 01:28:40PM +0200, Stefano Brivio wrote:
> > Given that pasta supports specifying a command to be executed on the
> > command line, even without the usual -- separator as long as there's
> > no ambiguity, we shouldn't eat up options that are not meant for us.
> > 
> > Paul reports, for instance, that with:
> > 
> >   pasta --config-net ip -6 route
> > 
> > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'.
> > That's because getopt_long(), by default, shuffles the argument list
> > to shift non-option arguments at the end.
> > 
> > Avoid that by adding '-' at the beginning of 'optstring', and mark the
> > position of the first non-option argument (getopt_long() will now
> > return the character code 1 once we hit it), so that we can use that
> > as command to run, or as PID for the target namespace.
> > 
> > Reported-by: Paul Holzinger <pholzing@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Eh... I'm kind of ambivalent about the idea.  I tend to think that
> accepting options in any position is generally expected behaviour, and
> anything programmatically adding commands to pasta should routinely
> insert a "--" (that's certainly what I do in testing code).

Sure, on the other hand it's convenient when you're quickly trying out
ip -6 route show, or some command where "--" would add 10% of typing or
so.

> But, that's not a particularly strong opinion, so whatever.
> 
> The implementation looks more complex than necessary, though.  AFAICT
> if you just add a '+' to the front of the optstring it will do exactly
> what you want without having to juggle the first_nonopt and other
> variables.

Oops, that's actually the first approach I considered, but I didn't see
any guarantee that optind would assume the expected value after the
loop. However, it does... sending v3.

-- 
Stefano

Re: [PATCH v2] conf: Stop parsing options at first non-option argument

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

On Thu, Aug 08, 2024 at 06:02:28AM +0200, Stefano Brivio wrote:
> On Thu, 8 Aug 2024 10:57:24 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Aug 07, 2024 at 01:28:40PM +0200, Stefano Brivio wrote:
> > > Given that pasta supports specifying a command to be executed on the
> > > command line, even without the usual -- separator as long as there's
> > > no ambiguity, we shouldn't eat up options that are not meant for us.
> > > 
> > > Paul reports, for instance, that with:
> > > 
> > >   pasta --config-net ip -6 route
> > > 
> > > -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'.
> > > That's because getopt_long(), by default, shuffles the argument list
> > > to shift non-option arguments at the end.
> > > 
> > > Avoid that by adding '-' at the beginning of 'optstring', and mark the
> > > position of the first non-option argument (getopt_long() will now
> > > return the character code 1 once we hit it), so that we can use that
> > > as command to run, or as PID for the target namespace.
> > > 
> > > Reported-by: Paul Holzinger <pholzing@redhat.com>
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > Eh... I'm kind of ambivalent about the idea.  I tend to think that
> > accepting options in any position is generally expected behaviour, and
> > anything programmatically adding commands to pasta should routinely
> > insert a "--" (that's certainly what I do in testing code).
> 
> Sure, on the other hand it's convenient when you're quickly trying out
> ip -6 route show, or some command where "--" would add 10% of typing or
> so.

Ok, sure.

> > But, that's not a particularly strong opinion, so whatever.
> > 
> > The implementation looks more complex than necessary, though.  AFAICT
> > if you just add a '+' to the front of the optstring it will do exactly
> > what you want without having to juggle the first_nonopt and other
> > variables.
> 
> Oops, that's actually the first approach I considered, but I didn't see
> any guarantee that optind would assume the expected value after the
> loop. However, it does... sending v3.

I don't really see how it could be anything else, after all you still
need to parse the remaining non-option arguments in this case.

-- 
David Gibson (he or they)	| 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 --]