Re: [PATCH v4 02/14] conf, fwd: Keep a table of our port forwarding configuration

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

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

On Fri, Jan 16, 2026 at 01:25:54AM +0100, Stefano Brivio wrote:
> On Fri, 16 Jan 2026 11:20:43 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jan 16, 2026 at 12:01:27AM +0100, Stefano Brivio wrote:
> > > On Thu, 15 Jan 2026 19:50:33 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > @@ -313,6 +330,90 @@ bool fwd_port_is_ephemeral(in_port_t port)
> > > >  	return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max);
> > > >  }
> > > >  
> > > > +/**
> > > > + * fwd_rule_add() - Add a rule to a forwarding table
> > > > + * @fwd:	Table to add to
> > > > + * @flags:	Flags for this entry
> > > > + * @addr:	Our address to forward (NULL for both 0.0.0.0 and ::)
> > > > + * @ifname:	Only forward from this interface name, if non-empty
> > > > + * @first:	First port number to forward
> > > > + * @last:	Last port number to forward
> > > > + * @to:		First port of target port range to map to
> > > > + */
> > > > +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> > > > +		  const union inany_addr *addr, const char *ifname,
> > > > +		  in_port_t first, in_port_t last, in_port_t to)
> > > > +{
> > > > +	/* Flags which can be set from the caller */
> > > > +	const uint8_t allowed_flags = FWD_WEAK;
> > > > +	struct fwd_rule *new;
> > > > +	unsigned port;
> > > > +
> > > > +	ASSERT(!(flags & ~allowed_flags));
> > > > +
> > > > +	if (fwd->count >= ARRAY_SIZE(fwd->rules))
> > > > +		die("Too many port forwarding ranges");
> > > > +
> > > > +	new = &fwd->rules[fwd->count++];
> > > > +	new->flags = flags;
> > > > +
> > > > +	if (addr) {
> > > > +		new->addr = *addr;
> > > > +	} else {
> > > > +		new->addr = inany_any6;
> > > > +		new->flags |= FWD_DUAL_STACK_ANY;
> > > > +	}
> > > > +
> > > > +	memset(new->ifname, 0, sizeof(new->ifname));
> > > > +	if (ifname) {
> > > > +		if (strlen(ifname) + 1 > sizeof(new->ifname))
> > > > +			die("Interface name %s is too long", ifname);
> > > > +		strncpy(new->ifname, ifname, sizeof(new->ifname));
> > > > +	}  
> > > 
> > > This looks safe to me now, but:
> > > 
> > > /home/sbrivio/passt/fwd.c:394:3:
> > >   Type: Buffer not null terminated (BUFFER_SIZE)  
> > [snip]
> > > ...perhaps worth switching to the usual snprintf() approach with return
> > > check (see handling of c->ip4.ifname_out in conf()) and be done with it?  
> > 
> > Good idea, not sure why it didn't occur to me earlier.
> > 
> > I've done that, and verified it fixes the coverity error (thanks for
> > resending the instructions for that).
> > 
> > > I'd be slightly more confident if Coverity Scan didn't complain at all
> > > (and happier without the noise, too).
> > > 
> > > Other than that, this version looks good to me. I would make a new
> > > release just before merging it (with this "fixed") so that we can debug
> > > things a bit more conveniently should something go wrong with it.  
> > 
> > That sounds wise.  Do you want a new spin with the coverity fix?  Just
> > this patch?  Something else?
> 
> Yes, thanks, a respin would be nice, so that we have a reasonable
> "permalink" to it, given it's a quite fundamental feature you're adding
> and we might want to refer to the series as posted / applied.

Ok. Will do that as soon as this test run completes.

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