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: 2822 bytes --]

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?

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