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

[Stefano Brivio <sbrivio@redhat.com>]

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)

/home/sbrivio/passt/fwd.c:354:2:
  1. path: Condition "!(flags & -7 /* ~allowed_flags */)", taking true branch.
/home/sbrivio/passt/fwd.c:356:2:
  2. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch.
/home/sbrivio/passt/fwd.c:358:2:
  3. path: Condition "fwd->sock_count + num > 196608U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch.
/home/sbrivio/passt/fwd.c:362:2:
  4. path: Condition "i < fwd->count", taking true branch.
/home/sbrivio/passt/fwd.c:366:3:
  5. path: Condition "!inany_matches(addr, fwd_rule_addr(rule))", taking false branch.
/home/sbrivio/passt/fwd.c:370:3:
  6. path: Condition "last < rule->first", taking true branch.
/home/sbrivio/passt/fwd.c:372:4:
  7. path: Continuing loop.
/home/sbrivio/passt/fwd.c:362:2:
  8. path: Condition "i < fwd->count", taking true branch.
/home/sbrivio/passt/fwd.c:366:3:
  9. path: Condition "!inany_matches(addr, fwd_rule_addr(rule))", taking false branch.
/home/sbrivio/passt/fwd.c:370:3:
  10. path: Condition "last < rule->first", taking true branch.
/home/sbrivio/passt/fwd.c:372:4:
  11. path: Continuing loop.
/home/sbrivio/passt/fwd.c:362:2:
  12. path: Condition "i < fwd->count", taking true branch.
/home/sbrivio/passt/fwd.c:366:3:
  13. path: Condition "!inany_matches(addr, fwd_rule_addr(rule))", taking false branch.
/home/sbrivio/passt/fwd.c:370:3:
  14. path: Condition "last < rule->first", taking false branch.
/home/sbrivio/passt/fwd.c:370:3:
  15. path: Condition "rule->last < first", taking true branch.
/home/sbrivio/passt/fwd.c:372:4:
  16. path: Continuing loop.
/home/sbrivio/passt/fwd.c:362:2:
  17. path: Condition "i < fwd->count", taking false branch.
/home/sbrivio/passt/fwd.c:383:2:
  18. path: Condition "addr", taking true branch.
/home/sbrivio/passt/fwd.c:385:2:
  19. path: Falling through to end of if statement.
/home/sbrivio/passt/fwd.c:391:2:
  20. path: Condition "ifname", taking true branch.
/home/sbrivio/passt/fwd.c:392:3:
  21. path: Condition "strlen(ifname) + 1 > 16UL /* sizeof (new->ifname) */", taking false branch.
/home/sbrivio/passt/fwd.c:394:3:
  22. buffer_size_warning: Calling "strncpy" with a maximum size argument of 16 bytes on destination array "new->ifname" of size 16 bytes might leave the destination string unterminated.
/home/sbrivio/passt/fwd.c:397:2:
  23. path: Condition "first <= last", taking true branch.
/home/sbrivio/passt/fwd.c:406:2:
  24. path: Condition "port <= new->last", taking true branch.
/home/sbrivio/passt/fwd.c:410:3:
  25. path: Condition "!(new->flags & (4UL /* 1UL << 2 */))", taking true branch.
/home/sbrivio/passt/fwd.c:412:2:
  26. path: Jumping back to the beginning of the loop.
/home/sbrivio/passt/fwd.c:406:2:
  27. path: Condition "port <= new->last", taking false branch.

...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?

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.

-- 
Stefano