Re: [PATCH v2 13/13] fwd_rule: Rewrite forward rule parsing using parse.c helpers

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Jul 02, 2026 at 01:12:00AM +0200, Stefano Brivio wrote:
> On Wed,  1 Jul 2026 15:31:55 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > +++ b/parse.c
> > @@ -18,6 +18,7 @@
> >  #include <string.h>
> >  #include <limits.h>
> >  #include <arpa/inet.h>
> > +#include <net/if.h>
> >  
> >  #include "common.h"
> >  #include "parse.h"
> > @@ -213,3 +214,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
> >  
> >  	return false;
> >  }
> > +
> > +/**
> > + * parse_ifspec() - Parse a interface name specifier (starting with %)
> > + * @ifname:	On success updated with parsed name (must have IFNAMSIZ space)
> > + *
> > + * This will accept a missing specifier (empty string), setting ifname to ""
> > + */
> > +bool parse_ifspec(const char **cursor, char *ifname)
> > +{
> > +	const char *p = *cursor;
> > +	size_t len;
> > +
> > +	if (!parse_literal(&p, "%")) {
> > +		/* No interface specifier */
> > +		ifname[0] = '\0';
> > +		return true;
> > +	}
> > +
> > +	/* ifnames can have anything that's not '/', '.' or whitespace */
> > +	len = strcspn(p, "/. \f\n\r\t\v");
> 
> I was about to apply the series when I spotted this: '.' is definitely
> valid in interface names, and even commonly used (think of VLANs), it's
> just "." and ".." _as an interface name_ that are not supported.

Oh, right, I misuderstood.  Should have looked at the kernel code
myself to confirm, oops.

> If we forbid any interface name with '.' we might very well break
> somebody's use case. I can fix this up with:
> 
> 	/* ifnames can't contain /, :, or whitespace */
> 	len = strcspn(p, "/: \f\n\r\t\v");
> 	if (!len || len >= IFNAMSIZ)
> 		return false;
> 
> 	/* ...and they can't be '..' or '.' either */
> 	if (!strcmp(p, ".") || !strcmp(p, ".."))
> 		return false;
> 
> if you're fine with it

That won't quite work: the string doesn't end at the end of the
interface name.  Plus, in my experience you get much better error
reporting if you generally try to lexically accept as *much* as
possible, then validate, rather than restricting the parser to only
accept the minimum it must.

I've added a validation of the parsed ifname up in fwd_rule_parse(),
instead.  I'll post the new spin momentarily.  .
> 
> > +	if (!len || len >= IFNAMSIZ)
> > +		return false;
> > +
> > +	memcpy(ifname, p, len);
> > +	ifname[len] = '\0';
> > +	*cursor = p + len;
> > +	return true;
> > +}
> > diff --git a/parse.h b/parse.h
> > index ab1d5adb..257b9c58 100644
> > --- a/parse.h
> > +++ b/parse.h
> > @@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
> >  
> >  #define parse_inany(cursor, addr)	parse_inany_((cursor), (addr), NULL)
> >  
> > +bool parse_ifspec(const char **cursor, char *ifname);
> > +
> >  #endif /* _PARSE_H */
> 
> -- 
> Stefano
> 

-- 
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 --]