[PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Ahelenia Ziemiańska]

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

The manual says
   COMMANDS
       These options specify the particular operation to perform.
       Only one of them can be specified at any given time.

       -L --dump
              List connection tracking or expectation table

So, naturally, "conntrack -Lo extended" should work,
but it doesn't, it's equivalent to "conntrack -L",
and you need "conntrack -L -o extended".
This violates user expectations (borne of the Utility Syntax Guidelines)
and contradicts the manual.

optarg is unused, anyway. Unclear why any of these were :: at all?

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 src/conntrack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index 0d71352..9fa4986 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -337,7 +337,7 @@ static struct option original_opts[] = {
 	{0, 0, 0, 0}
 };
 
-static const char *getopt_str = ":L::I::U::D::G::E::F::A::hVs:d:r:q:"
+static const char *getopt_str = ":LIUDGEFAhVs:d:r:q:"
 				"p:t:u:e:a:z[:]:{:}:m:i:f:o:n::"
 				"g::c:b:C::Sj::w:l:<:>::(:):";
 
-- 
2.39.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Pablo Neira Ayuso]

On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> The manual says
>    COMMANDS
>        These options specify the particular operation to perform.
>        Only one of them can be specified at any given time.
> 
>        -L --dump
>               List connection tracking or expectation table
> 
> So, naturally, "conntrack -Lo extended" should work,
> but it doesn't, it's equivalent to "conntrack -L",
> and you need "conntrack -L -o extended".
> This violates user expectations (borne of the Utility Syntax Guidelines)
> and contradicts the manual.
> 
> optarg is unused, anyway. Unclear why any of these were :: at all?

Because this supports:

        -L
        -L conntrack
        -L expect

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Ahelenia Ziemiańska]

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

On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > The manual says
> >    COMMANDS
> >        These options specify the particular operation to perform.
> >        Only one of them can be specified at any given time.
> > 
> >        -L --dump
> >               List connection tracking or expectation table
> > 
> > So, naturally, "conntrack -Lo extended" should work,
> > but it doesn't, it's equivalent to "conntrack -L",
> > and you need "conntrack -L -o extended".
> > This violates user expectations (borne of the Utility Syntax Guidelines)
> > and contradicts the manual.
> > 
> > optarg is unused, anyway. Unclear why any of these were :: at all?
> Because this supports:
>         -L
>         -L conntrack
>         -L expect
Well that's not what :: does, though; we realise this, right?

"L::" means that getopt() will return
  "-L", "conntrack" -> 'L',optarg=NULL
  "-Lconntrack"     -> 'L',optarg="conntrack"
and the parser for -L (&c.) doesn't... use optarg.

You don't parse the filter (table name? idk.) with getopt at all;
you can test this /right now/ by running precisely the thing you outlined:
  # conntrack -L > /dev/null
  conntrack v1.4.7 (conntrack-tools): 137 flow entries have been shown.
  # conntrack -L expect > /dev/null
  conntrack v1.4.7 (conntrack-tools): 0 expectations have been shown.
  # conntrack -Lexpect > /dev/null
  conntrack v1.4.7 (conntrack-tools): 152 flow entries have been shown.
and getopt returns, respectively
  'L',optarg=NULL
  'L',optarg=NULL; argv[optind]="expect"
  'L',optarg="expect"
...and once again you discard the optarg for 'L' &c.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Pablo Neira Ayuso]

On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > The manual says
> > >    COMMANDS
> > >        These options specify the particular operation to perform.
> > >        Only one of them can be specified at any given time.
> > > 
> > >        -L --dump
> > >               List connection tracking or expectation table
> > > 
> > > So, naturally, "conntrack -Lo extended" should work,
> > > but it doesn't, it's equivalent to "conntrack -L",
> > > and you need "conntrack -L -o extended".
> > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > and contradicts the manual.
> > > 
> > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > Because this supports:
> >         -L
> >         -L conntrack
> >         -L expect
> Well that's not what :: does, though; we realise this, right?
> 
> "L::" means that getopt() will return
>   "-L", "conntrack" -> 'L',optarg=NULL
>   "-Lconntrack"     -> 'L',optarg="conntrack"
> and the parser for -L (&c.) doesn't... use optarg.
> 
> You don't parse the filter (table name? idk.) with getopt at all;
> you can test this /right now/ by running precisely the thing you outlined:
>   # conntrack -L > /dev/null
>   conntrack v1.4.7 (conntrack-tools): 137 flow entries have been shown.
>   # conntrack -L expect > /dev/null
>   conntrack v1.4.7 (conntrack-tools): 0 expectations have been shown.
>   # conntrack -Lexpect > /dev/null
>   conntrack v1.4.7 (conntrack-tools): 152 flow entries have been shown.
> and getopt returns, respectively
>   'L',optarg=NULL
>   'L',optarg=NULL; argv[optind]="expect"
>   'L',optarg="expect"
> ...and once again you discard the optarg for 'L' &c.

Your evaluation is correct, patch is applied, thanks.

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Pablo Neira Ayuso]

On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > The manual says
> > >    COMMANDS
> > >        These options specify the particular operation to perform.
> > >        Only one of them can be specified at any given time.
> > > 
> > >        -L --dump
> > >               List connection tracking or expectation table
> > > 
> > > So, naturally, "conntrack -Lo extended" should work,
> > > but it doesn't, it's equivalent to "conntrack -L",
> > > and you need "conntrack -L -o extended".
> > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > and contradicts the manual.
> > > 
> > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > Because this supports:
> >         -L
> >         -L conntrack
> >         -L expect
> Well that's not what :: does, though; we realise this, right?
> 
> "L::" means that getopt() will return
>   "-L", "conntrack" -> 'L',optarg=NULL
>   "-Lconntrack"     -> 'L',optarg="conntrack"
> and the parser for -L (&c.) doesn't... use optarg.

Are you sure it does not use optarg?

static unsigned int check_type(int argc, char *argv[])
{
        const char *table = get_optional_arg(argc, argv);

and get_optional_arg() uses optarg.

> You don't parse the filter (table name? idk.) with getopt at all;
> you can test this /right now/ by running precisely the thing you outlined:
>   # conntrack -L > /dev/null
>   conntrack v1.4.7 (conntrack-tools): 137 flow entries have been shown.
>   # conntrack -L expect > /dev/null
>   conntrack v1.4.7 (conntrack-tools): 0 expectations have been shown.
>   # conntrack -Lexpect > /dev/null
>   conntrack v1.4.7 (conntrack-tools): 152 flow entries have been shown.
> and getopt returns, respectively
>   'L',optarg=NULL
>   'L',optarg=NULL; argv[optind]="expect"
>   'L',optarg="expect"
> ...and once again you discard the optarg for 'L' &c.

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Ahelenia Ziemiańska]

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

On Wed, Sep 25, 2024 at 04:53:49PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> > On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > > The manual says
> > > >    COMMANDS
> > > >        These options specify the particular operation to perform.
> > > >        Only one of them can be specified at any given time.
> > > > 
> > > >        -L --dump
> > > >               List connection tracking or expectation table
> > > > 
> > > > So, naturally, "conntrack -Lo extended" should work,
> > > > but it doesn't, it's equivalent to "conntrack -L",
> > > > and you need "conntrack -L -o extended".
> > > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > > and contradicts the manual.
> > > > 
> > > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > > Because this supports:
> > >         -L
> > >         -L conntrack
> > >         -L expect
> > Well that's not what :: does, though; we realise this, right?
> > 
> > "L::" means that getopt() will return
> >   "-L", "conntrack" -> 'L',optarg=NULL
> >   "-Lconntrack"     -> 'L',optarg="conntrack"
> > and the parser for -L (&c.) doesn't... use optarg.
> Are you sure it does not use optarg?
> 
> static unsigned int check_type(int argc, char *argv[])
> {
>         const char *table = get_optional_arg(argc, argv);
> 
> and get_optional_arg() uses optarg.

This I've missed, but actually my diagnosis still holds:
  static unsigned int check_type(int argc, char *argv[])
  {
  	const char *table = get_optional_arg(argc, argv);
  
  	/* default to conntrack subsystem if nothing has been specified. */
  	if (table == NULL)
  		return CT_TABLE_CONNTRACK;

  static char *get_optional_arg(int argc, char *argv[])
  {
  	char *arg = NULL;
  
  	/* Nasty bug or feature in getopt_long ?
  	 * It seems that it behaves badly with optional arguments.
  	 * Fortunately, I just stole the fix from iptables ;) */
  	if (optarg)
  		return arg;

So, if you say -Lanything, then
  optarg=anything
  get_optional_arg=(null)
(notice that it says "return arg;", not "return optarg;",
 i.e. this is "return NULL").

It /doesn't/ use optarg, because it explicitly treats an optarg as no optarg.

It's unclear to me what the comment is referencing,
but I'm assuming some sort of confusion with what :: does?
Anyway, that if(){ can be removed now, since it can never be taken now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Pablo Neira Ayuso]

On Wed, Sep 25, 2024 at 05:11:01PM +0200, Ahelenia Ziemiańska wrote:
> On Wed, Sep 25, 2024 at 04:53:49PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> > > On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > > > The manual says
> > > > >    COMMANDS
> > > > >        These options specify the particular operation to perform.
> > > > >        Only one of them can be specified at any given time.
> > > > > 
> > > > >        -L --dump
> > > > >               List connection tracking or expectation table
> > > > > 
> > > > > So, naturally, "conntrack -Lo extended" should work,
> > > > > but it doesn't, it's equivalent to "conntrack -L",
> > > > > and you need "conntrack -L -o extended".
> > > > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > > > and contradicts the manual.
> > > > > 
> > > > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > > > Because this supports:
> > > >         -L
> > > >         -L conntrack
> > > >         -L expect
> > > Well that's not what :: does, though; we realise this, right?
> > > 
> > > "L::" means that getopt() will return
> > >   "-L", "conntrack" -> 'L',optarg=NULL
> > >   "-Lconntrack"     -> 'L',optarg="conntrack"
> > > and the parser for -L (&c.) doesn't... use optarg.
> > Are you sure it does not use optarg?
> > 
> > static unsigned int check_type(int argc, char *argv[])
> > {
> >         const char *table = get_optional_arg(argc, argv);
> > 
> > and get_optional_arg() uses optarg.
> 
> This I've missed, but actually my diagnosis still holds:
>   static unsigned int check_type(int argc, char *argv[])
>   {
>   	const char *table = get_optional_arg(argc, argv);
>   
>   	/* default to conntrack subsystem if nothing has been specified. */
>   	if (table == NULL)
>   		return CT_TABLE_CONNTRACK;
> 
>   static char *get_optional_arg(int argc, char *argv[])
>   {
>   	char *arg = NULL;
>   
>   	/* Nasty bug or feature in getopt_long ?
>   	 * It seems that it behaves badly with optional arguments.
>   	 * Fortunately, I just stole the fix from iptables ;) */
>   	if (optarg)
>   		return arg;
> 
> So, if you say -Lanything, then
>   optarg=anything
>   get_optional_arg=(null)
> (notice that it says "return arg;", not "return optarg;",
>  i.e. this is "return NULL").
> 
> It /doesn't/ use optarg, because it explicitly treats an optarg as no optarg.
> 
> It's unclear to me what the comment is referencing,
> but I'm assuming some sort of confusion with what :: does?
> Anyway, that if(){ can be removed now, since it can never be taken now.

The issue that I'm observing is that

# conntrack -Lconntrack

now optarg is NULL after your patch, so 'conntrack' is ignored, so it
falls back to list the conntrack table.

Then, this breaks:

# conntrack -Lexpect
conntrack v1.4.9 (conntrack-tools): Bad parameter `xpect'
Try `conntrack -h' or 'conntrack --help' for more information.

Maybe your patch needs an extension to deal with this case too?

Regarding your question, this parser is old and I shamelessly took it
from the original iptables to make syntax similar.

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[наб]

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

On Wed, Sep 25, 2024 at 10:32:59PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 25, 2024 at 05:11:01PM +0200, Ahelenia Ziemiańska wrote:
> > On Wed, Sep 25, 2024 at 04:53:49PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> > > > On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > > > > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > > > > The manual says
> > > > > >    COMMANDS
> > > > > >        These options specify the particular operation to perform.
> > > > > >        Only one of them can be specified at any given time.
> > > > > > 
> > > > > >        -L --dump
> > > > > >               List connection tracking or expectation table
> > > > > > 
> > > > > > So, naturally, "conntrack -Lo extended" should work,
> > > > > > but it doesn't, it's equivalent to "conntrack -L",
> > > > > > and you need "conntrack -L -o extended".
> > > > > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > > > > and contradicts the manual.
> > > > > > 
> > > > > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > > > > Because this supports:
> > > > >         -L
> > > > >         -L conntrack
> > > > >         -L expect
> > > > Well that's not what :: does, though; we realise this, right?
> > > > 
> > > > "L::" means that getopt() will return
> > > >   "-L", "conntrack" -> 'L',optarg=NULL
> > > >   "-Lconntrack"     -> 'L',optarg="conntrack"
> > > > and the parser for -L (&c.) doesn't... use optarg.
> > > Are you sure it does not use optarg?
> > > 
> > > static unsigned int check_type(int argc, char *argv[])
> > > {
> > >         const char *table = get_optional_arg(argc, argv);
> > > 
> > > and get_optional_arg() uses optarg.
> > This I've missed, but actually my diagnosis still holds:
> >   static unsigned int check_type(int argc, char *argv[])
> >   {
> >   	const char *table = get_optional_arg(argc, argv);
> >   
> >   	/* default to conntrack subsystem if nothing has been specified. */
> >   	if (table == NULL)
> >   		return CT_TABLE_CONNTRACK;
> > 
> >   static char *get_optional_arg(int argc, char *argv[])
> >   {
> >   	char *arg = NULL;
> >   
> >   	/* Nasty bug or feature in getopt_long ?
> >   	 * It seems that it behaves badly with optional arguments.
> >   	 * Fortunately, I just stole the fix from iptables ;) */
> >   	if (optarg)
> >   		return arg;
> > 
> > So, if you say -Lanything, then
> >   optarg=anything
> >   get_optional_arg=(null)
> > (notice that it says "return arg;", not "return optarg;",
> >  i.e. this is "return NULL").
> > 
> > It /doesn't/ use optarg, because it explicitly treats an optarg as no optarg.
> > 
> > It's unclear to me what the comment is referencing,
> > but I'm assuming some sort of confusion with what :: does?
> > Anyway, that if(){ can be removed now, since it can never be taken now.
> Then, this breaks:
> # conntrack -Lexpect
> conntrack v1.4.9 (conntrack-tools): Bad parameter `xpect'
> Try `conntrack -h' or 'conntrack --help' for more information.
> 
> Maybe your patch needs an extension to deal with this case too?

This doesn't "break", this is equivalent to conntrack -L -e xpect.
It's now correct. This was the crux of the patch, actually.

Compare the manual:
  SYNOPSIS
    conntrack -L [table] [options] [-z]
  COMMANDS
    -L --dump     List connection tracking or expectation table
  PARAMETERS
    -e, --event-mask [ALL|NEW|UPDATES|DESTROY][,...]
                  Set the bitmask of events that are to be generated by the in-kernel ctnetlink event code.  Using this parameter, you can reduce the event messages  generated
                  by the kernel to the types that you are actually interested in.  This option can only be used in conjunction with "-E, --event".

Previously, it /was/ broken: conntrack -Lexpect was as-if --dump=expect
(also not legal since --dump doesn't take an argument),
and the "expect" was ignored, so it was equivalent to conntrack -L.
You can trivially validate this by running an older version.

(Well, --dump=expect /is/ accepted. And ignored.
 So fix that too with s/optional_argument/no_argument/ (or s/2/0/).
 I didn't actually look at the longopts before.)

> The issue that I'm observing is that
>   # conntrack -Lconntrack
> now optarg is NULL after your patch, so 'conntrack' is ignored, so it
> falls back to list the conntrack table.

What do you mean "now". That shit was always ignored.
You can read trace the calls yourself if you don't believe my analysis.
Now it behaves as-documented (-L -c onntrack).

And, per
                case 'c':
                        options |= opt2type[c];
                        nfct_set_attr_u32(tmpl->ct,
                                          opt2attr[c],
                                          strtoul(optarg, NULL, 0));
                        break;
-c onntrack is equivalent to -c 0.
This is also obviously wrong.

I will repeat this and you can confirm this once more
(or refer back to my analysis above):
for all of -LIUDGEFA, an optional parameter was accepted, and always discarded.
It now isn't, and behaves as-expected per the USG
("the USG" is an annoying way to say "how getopt() works".

> Regarding your question, this parser is old and I shamelessly took it
> from the original iptables to make syntax similar.
So you have someone to blame it on when it turns out to be dysfunctional.
But you also have a huge parser that doesn't work.
Win some/lose some, I suppose.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Pablo Neira Ayuso]

On Thu, Sep 26, 2024 at 10:28:58AM +0200, наб wrote:
> On Wed, Sep 25, 2024 at 10:32:59PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 25, 2024 at 05:11:01PM +0200, Ahelenia Ziemiańska wrote:
> > > On Wed, Sep 25, 2024 at 04:53:49PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> > > > > On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > > > > > The manual says
> > > > > > >    COMMANDS
> > > > > > >        These options specify the particular operation to perform.
> > > > > > >        Only one of them can be specified at any given time.
> > > > > > > 
> > > > > > >        -L --dump
> > > > > > >               List connection tracking or expectation table
> > > > > > > 
> > > > > > > So, naturally, "conntrack -Lo extended" should work,
> > > > > > > but it doesn't, it's equivalent to "conntrack -L",
> > > > > > > and you need "conntrack -L -o extended".
> > > > > > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > > > > > and contradicts the manual.
> > > > > > > 
> > > > > > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > > > > > Because this supports:
> > > > > >         -L
> > > > > >         -L conntrack
> > > > > >         -L expect
> > > > > Well that's not what :: does, though; we realise this, right?
> > > > > 
> > > > > "L::" means that getopt() will return
> > > > >   "-L", "conntrack" -> 'L',optarg=NULL
> > > > >   "-Lconntrack"     -> 'L',optarg="conntrack"
> > > > > and the parser for -L (&c.) doesn't... use optarg.
> > > > Are you sure it does not use optarg?
> > > > 
> > > > static unsigned int check_type(int argc, char *argv[])
> > > > {
> > > >         const char *table = get_optional_arg(argc, argv);
> > > > 
> > > > and get_optional_arg() uses optarg.
> > > This I've missed, but actually my diagnosis still holds:
> > >   static unsigned int check_type(int argc, char *argv[])
> > >   {
> > >   	const char *table = get_optional_arg(argc, argv);
> > >   
> > >   	/* default to conntrack subsystem if nothing has been specified. */
> > >   	if (table == NULL)
> > >   		return CT_TABLE_CONNTRACK;
> > > 
> > >   static char *get_optional_arg(int argc, char *argv[])
> > >   {
> > >   	char *arg = NULL;
> > >   
> > >   	/* Nasty bug or feature in getopt_long ?
> > >   	 * It seems that it behaves badly with optional arguments.
> > >   	 * Fortunately, I just stole the fix from iptables ;) */
> > >   	if (optarg)
> > >   		return arg;
> > > 
> > > So, if you say -Lanything, then
> > >   optarg=anything
> > >   get_optional_arg=(null)
> > > (notice that it says "return arg;", not "return optarg;",
> > >  i.e. this is "return NULL").
> > > 
> > > It /doesn't/ use optarg, because it explicitly treats an optarg as no optarg.
> > > 
> > > It's unclear to me what the comment is referencing,
> > > but I'm assuming some sort of confusion with what :: does?
> > > Anyway, that if(){ can be removed now, since it can never be taken now.
> > Then, this breaks:
> > # conntrack -Lexpect
> > conntrack v1.4.9 (conntrack-tools): Bad parameter `xpect'
> > Try `conntrack -h' or 'conntrack --help' for more information.
> > 
> > Maybe your patch needs an extension to deal with this case too?
> 
> This doesn't "break", this is equivalent to conntrack -L -e xpect.
> It's now correct. This was the crux of the patch, actually.
> 
> Compare the manual:
>   SYNOPSIS
>     conntrack -L [table] [options] [-z]
>   COMMANDS
>     -L --dump     List connection tracking or expectation table
>   PARAMETERS
>     -e, --event-mask [ALL|NEW|UPDATES|DESTROY][,...]
>                   Set the bitmask of events that are to be generated by the in-kernel ctnetlink event code.  Using this parameter, you can reduce the event messages  generated
>                   by the kernel to the types that you are actually interested in.  This option can only be used in conjunction with "-E, --event".
> 
> Previously, it /was/ broken: conntrack -Lexpect was as-if --dump=expect
> (also not legal since --dump doesn't take an argument),
> and the "expect" was ignored, so it was equivalent to conntrack -L.
> You can trivially validate this by running an older version.
> 
> (Well, --dump=expect /is/ accepted. And ignored.
>  So fix that too with s/optional_argument/no_argument/ (or s/2/0/).
>  I didn't actually look at the longopts before.)
> 
> > The issue that I'm observing is that
> >   # conntrack -Lconntrack
> > now optarg is NULL after your patch, so 'conntrack' is ignored, so it
> > falls back to list the conntrack table.
> 
> What do you mean "now". That shit was always ignored.
> You can read trace the calls yourself if you don't believe my analysis.
> Now it behaves as-documented (-L -c onntrack).
> 
> And, per
>                 case 'c':
>                         options |= opt2type[c];
>                         nfct_set_attr_u32(tmpl->ct,
>                                           opt2attr[c],
>                                           strtoul(optarg, NULL, 0));
>                         break;
> -c onntrack is equivalent to -c 0.
> This is also obviously wrong.
> 
> I will repeat this and you can confirm this once more
> (or refer back to my analysis above):
> for all of -LIUDGEFA, an optional parameter was accepted, and always discarded.
> It now isn't, and behaves as-expected per the USG
> ("the USG" is an annoying way to say "how getopt() works".
> 
> > Regarding your question, this parser is old and I shamelessly took it
> > from the original iptables to make syntax similar.
> So you have someone to blame it on when it turns out to be dysfunctional.
> But you also have a huge parser that doesn't work.
> Win some/lose some, I suppose.

Your stuff breaks existing behaviour. I will revert and leave it as is.

There is a risk of breaking existing applications.

You can use the word shit, dysfunctional, and keep augment your
wording as many times as you want, but that does not change my point.

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[Pablo Neira Ayuso]

On Thu, Sep 26, 2024 at 12:33:00PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 26, 2024 at 10:28:58AM +0200, наб wrote:
> > On Wed, Sep 25, 2024 at 10:32:59PM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, Sep 25, 2024 at 05:11:01PM +0200, Ahelenia Ziemiańska wrote:
> > > > On Wed, Sep 25, 2024 at 04:53:49PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> > > > > > On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > > > > > > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > > > > > > The manual says
> > > > > > > >    COMMANDS
> > > > > > > >        These options specify the particular operation to perform.
> > > > > > > >        Only one of them can be specified at any given time.
> > > > > > > > 
> > > > > > > >        -L --dump
> > > > > > > >               List connection tracking or expectation table
> > > > > > > > 
> > > > > > > > So, naturally, "conntrack -Lo extended" should work,
> > > > > > > > but it doesn't, it's equivalent to "conntrack -L",
> > > > > > > > and you need "conntrack -L -o extended".
> > > > > > > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > > > > > > and contradicts the manual.
> > > > > > > > 
> > > > > > > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > > > > > > Because this supports:
> > > > > > >         -L
> > > > > > >         -L conntrack
> > > > > > >         -L expect
> > > > > > Well that's not what :: does, though; we realise this, right?
> > > > > > 
> > > > > > "L::" means that getopt() will return
> > > > > >   "-L", "conntrack" -> 'L',optarg=NULL
> > > > > >   "-Lconntrack"     -> 'L',optarg="conntrack"
> > > > > > and the parser for -L (&c.) doesn't... use optarg.
> > > > > Are you sure it does not use optarg?
> > > > > 
> > > > > static unsigned int check_type(int argc, char *argv[])
> > > > > {
> > > > >         const char *table = get_optional_arg(argc, argv);
> > > > > 
> > > > > and get_optional_arg() uses optarg.
> > > > This I've missed, but actually my diagnosis still holds:
> > > >   static unsigned int check_type(int argc, char *argv[])
> > > >   {
> > > >   	const char *table = get_optional_arg(argc, argv);
> > > >   
> > > >   	/* default to conntrack subsystem if nothing has been specified. */
> > > >   	if (table == NULL)
> > > >   		return CT_TABLE_CONNTRACK;
> > > > 
> > > >   static char *get_optional_arg(int argc, char *argv[])
> > > >   {
> > > >   	char *arg = NULL;
> > > >   
> > > >   	/* Nasty bug or feature in getopt_long ?
> > > >   	 * It seems that it behaves badly with optional arguments.
> > > >   	 * Fortunately, I just stole the fix from iptables ;) */
> > > >   	if (optarg)
> > > >   		return arg;
> > > > 
> > > > So, if you say -Lanything, then
> > > >   optarg=anything
> > > >   get_optional_arg=(null)
> > > > (notice that it says "return arg;", not "return optarg;",
> > > >  i.e. this is "return NULL").
> > > > 
> > > > It /doesn't/ use optarg, because it explicitly treats an optarg as no optarg.
> > > > 
> > > > It's unclear to me what the comment is referencing,
> > > > but I'm assuming some sort of confusion with what :: does?
> > > > Anyway, that if(){ can be removed now, since it can never be taken now.
> > > Then, this breaks:
> > > # conntrack -Lexpect
> > > conntrack v1.4.9 (conntrack-tools): Bad parameter `xpect'
> > > Try `conntrack -h' or 'conntrack --help' for more information.
> > > 
> > > Maybe your patch needs an extension to deal with this case too?
> > 
> > This doesn't "break", this is equivalent to conntrack -L -e xpect.
> > It's now correct. This was the crux of the patch, actually.
> > 
> > Compare the manual:
> >   SYNOPSIS
> >     conntrack -L [table] [options] [-z]
> >   COMMANDS
> >     -L --dump     List connection tracking or expectation table
> >   PARAMETERS
> >     -e, --event-mask [ALL|NEW|UPDATES|DESTROY][,...]
> >                   Set the bitmask of events that are to be generated by the in-kernel ctnetlink event code.  Using this parameter, you can reduce the event messages  generated
> >                   by the kernel to the types that you are actually interested in.  This option can only be used in conjunction with "-E, --event".
> > 
> > Previously, it /was/ broken: conntrack -Lexpect was as-if --dump=expect
> > (also not legal since --dump doesn't take an argument),
> > and the "expect" was ignored, so it was equivalent to conntrack -L.
> > You can trivially validate this by running an older version.
> > 
> > (Well, --dump=expect /is/ accepted. And ignored.
> >  So fix that too with s/optional_argument/no_argument/ (or s/2/0/).
> >  I didn't actually look at the longopts before.)
> > 
> > > The issue that I'm observing is that
> > >   # conntrack -Lconntrack
> > > now optarg is NULL after your patch, so 'conntrack' is ignored, so it
> > > falls back to list the conntrack table.
> > 
> > What do you mean "now". That shit was always ignored.
> > You can read trace the calls yourself if you don't believe my analysis.
> > Now it behaves as-documented (-L -c onntrack).
> > 
> > And, per
> >                 case 'c':
> >                         options |= opt2type[c];
> >                         nfct_set_attr_u32(tmpl->ct,
> >                                           opt2attr[c],
> >                                           strtoul(optarg, NULL, 0));
> >                         break;
> > -c onntrack is equivalent to -c 0.
> > This is also obviously wrong.
> > 
> > I will repeat this and you can confirm this once more
> > (or refer back to my analysis above):
> > for all of -LIUDGEFA, an optional parameter was accepted, and always discarded.
> > It now isn't, and behaves as-expected per the USG
> > ("the USG" is an annoying way to say "how getopt() works".
> > 
> > > Regarding your question, this parser is old and I shamelessly took it
> > > from the original iptables to make syntax similar.
> > So you have someone to blame it on when it turns out to be dysfunctional.
> > But you also have a huge parser that doesn't work.
> > Win some/lose some, I suppose.
> 
> Your stuff breaks existing behaviour. I will revert and leave it as is.
> 
> There is a risk of breaking existing applications.
> 
> You can use the word shit, dysfunctional, and keep augment your
> wording as many times as you want, but that does not change my point.

So either fix it is a backward compatible way or there will be no fix.

Re: [PATCH] conntrack: -L doesn't take a value, so don't discard one (same for -IUDGEFA)

[наб]

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

On Thu, Sep 26, 2024 at 12:38:08PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 26, 2024 at 12:33:00PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Sep 26, 2024 at 10:28:58AM +0200, наб wrote:
> > > On Wed, Sep 25, 2024 at 10:32:59PM +0200, Pablo Neira Ayuso wrote:
> > > > On Wed, Sep 25, 2024 at 05:11:01PM +0200, Ahelenia Ziemiańska wrote:
> > > > > On Wed, Sep 25, 2024 at 04:53:49PM +0200, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Sep 03, 2024 at 04:53:46PM +0200, Ahelenia Ziemiańska wrote:
> > > > > > > On Tue, Sep 03, 2024 at 10:22:09AM +0200, Pablo Neira Ayuso wrote:
> > > > > > > > On Tue, Sep 03, 2024 at 04:16:21AM +0200, Ahelenia Ziemiańska wrote:
> > > > > > > > > The manual says
> > > > > > > > >    COMMANDS
> > > > > > > > >        These options specify the particular operation to perform.
> > > > > > > > >        Only one of them can be specified at any given time.
> > > > > > > > > 
> > > > > > > > >        -L --dump
> > > > > > > > >               List connection tracking or expectation table
> > > > > > > > > 
> > > > > > > > > So, naturally, "conntrack -Lo extended" should work,
> > > > > > > > > but it doesn't, it's equivalent to "conntrack -L",
> > > > > > > > > and you need "conntrack -L -o extended".
> > > > > > > > > This violates user expectations (borne of the Utility Syntax Guidelines)
> > > > > > > > > and contradicts the manual.
> > > > > > > > > 
> > > > > > > > > optarg is unused, anyway. Unclear why any of these were :: at all?
> > > > > > > > Because this supports:
> > > > > > > >         -L
> > > > > > > >         -L conntrack
> > > > > > > >         -L expect
> > > > > > > Well that's not what :: does, though; we realise this, right?
> > > > > > > 
> > > > > > > "L::" means that getopt() will return
> > > > > > >   "-L", "conntrack" -> 'L',optarg=NULL
> > > > > > >   "-Lconntrack"     -> 'L',optarg="conntrack"
> > > > > > > and the parser for -L (&c.) doesn't... use optarg.
> > > > > > Are you sure it does not use optarg?
> > > > > > 
> > > > > > static unsigned int check_type(int argc, char *argv[])
> > > > > > {
> > > > > >         const char *table = get_optional_arg(argc, argv);
> > > > > > 
> > > > > > and get_optional_arg() uses optarg.
> > > > > This I've missed, but actually my diagnosis still holds:
> > > > >   static unsigned int check_type(int argc, char *argv[])
> > > > >   {
> > > > >   	const char *table = get_optional_arg(argc, argv);
> > > > >   
> > > > >   	/* default to conntrack subsystem if nothing has been specified. */
> > > > >   	if (table == NULL)
> > > > >   		return CT_TABLE_CONNTRACK;
> > > > > 
> > > > >   static char *get_optional_arg(int argc, char *argv[])
> > > > >   {
> > > > >   	char *arg = NULL;
> > > > >   
> > > > >   	/* Nasty bug or feature in getopt_long ?
> > > > >   	 * It seems that it behaves badly with optional arguments.
> > > > >   	 * Fortunately, I just stole the fix from iptables ;) */
> > > > >   	if (optarg)
> > > > >   		return arg;
> > > > > 
> > > > > So, if you say -Lanything, then
> > > > >   optarg=anything
> > > > >   get_optional_arg=(null)
> > > > > (notice that it says "return arg;", not "return optarg;",
> > > > >  i.e. this is "return NULL").
> > > > > 
> > > > > It /doesn't/ use optarg, because it explicitly treats an optarg as no optarg.
> > > > > 
> > > > > It's unclear to me what the comment is referencing,
> > > > > but I'm assuming some sort of confusion with what :: does?
> > > > > Anyway, that if(){ can be removed now, since it can never be taken now.
> > > > Then, this breaks:
> > > > # conntrack -Lexpect
> > > > conntrack v1.4.9 (conntrack-tools): Bad parameter `xpect'
> > > > Try `conntrack -h' or 'conntrack --help' for more information.
> > > > 
> > > > Maybe your patch needs an extension to deal with this case too?
> > > This doesn't "break", this is equivalent to conntrack -L -e xpect.
> > > It's now correct. This was the crux of the patch, actually.
> > > 
> > > Compare the manual:
> > >   SYNOPSIS
> > >     conntrack -L [table] [options] [-z]
> > >   COMMANDS
> > >     -L --dump     List connection tracking or expectation table
> > >   PARAMETERS
> > >     -e, --event-mask [ALL|NEW|UPDATES|DESTROY][,...]
> > >                   Set the bitmask of events that are to be generated by the in-kernel ctnetlink event code.  Using this parameter, you can reduce the event messages  generated
> > >                   by the kernel to the types that you are actually interested in.  This option can only be used in conjunction with "-E, --event".
> > > 
> > > Previously, it /was/ broken: conntrack -Lexpect was as-if --dump=expect
> > > (also not legal since --dump doesn't take an argument),
> > > and the "expect" was ignored, so it was equivalent to conntrack -L.
> > > You can trivially validate this by running an older version.
> > > 
> > > (Well, --dump=expect /is/ accepted. And ignored.
> > >  So fix that too with s/optional_argument/no_argument/ (or s/2/0/).
> > >  I didn't actually look at the longopts before.)
> > > 
> > > > The issue that I'm observing is that
> > > >   # conntrack -Lconntrack
> > > > now optarg is NULL after your patch, so 'conntrack' is ignored, so it
> > > > falls back to list the conntrack table.
> > > 
> > > What do you mean "now". That shit was always ignored.
> > > You can read trace the calls yourself if you don't believe my analysis.
> > > Now it behaves as-documented (-L -c onntrack).
> > > 
> > > And, per
> > >                 case 'c':
> > >                         options |= opt2type[c];
> > >                         nfct_set_attr_u32(tmpl->ct,
> > >                                           opt2attr[c],
> > >                                           strtoul(optarg, NULL, 0));
> > >                         break;
> > > -c onntrack is equivalent to -c 0.
> > > This is also obviously wrong.
> > > 
> > > I will repeat this and you can confirm this once more
> > > (or refer back to my analysis above):
> > > for all of -LIUDGEFA, an optional parameter was accepted, and always discarded.
> > > It now isn't, and behaves as-expected per the USG
> > > ("the USG" is an annoying way to say "how getopt() works".
> > > 
> > > > Regarding your question, this parser is old and I shamelessly took it
> > > > from the original iptables to make syntax similar.
> > > So you have someone to blame it on when it turns out to be dysfunctional.
> > > But you also have a huge parser that doesn't work.
> > > Win some/lose some, I suppose.
> > Your stuff breaks existing behaviour. I will revert and leave it as is.
> > 
> > There is a risk of breaking existing applications.
Hardly, since the behaviour you're trying to preserve is undocumented
(well, actually runs counter to your documentation)
and currently does nothing.

> > You can use the word shit, dysfunctional, and keep augment your
> > wording as many times as you want, but that does not change my point.
AFAICT so far your point was never that you believe that the current
behaviour is correct, and I thought it impossible to argue this.

> So either fix it is a backward compatible way or there will be no fix.
If you don't intend to fix this parser
(defined as "don't intend to make it behave like you document it")
then ship documentation that accurately describes it.
That should be simple enough,
provided you know what it does and what behaviour you're preserving.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]