Re: [PATCH v3 3/5] treewide: Don't rely on terminator records in ip[46].dns arrays

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

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

On Sun, Jan 11, 2026 at 12:33:00AM +0100, Stefano Brivio wrote:
> On Wed,  7 Jan 2026 12:46:04 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In our arrays of DNS resolvers to pass to the guest we use a blank entry
> > to indicate the end of the list.  We rely on this when scanning the array,
> > not having separate bounds checking.  clang-tidy 21.1.7 has fancier
> > checking for array overruns in loops, but it's not able to reason that
> > there's always a terminating entry, so complains.
> > 
> > Indeed, it's correct to do so in this case.  Although we allow space in the
> > arrays for the terminator (size MAXNS + 1), add_dns[46]() check only for
> >     idx >= ARRAY_SIZE()
> > before adding an entry.  This allows it to consume the last slot with a
> > "real" entry, meaning the places where we scan really could overrun.
> > 
> > Fix the bug, and make it easier to reason about (for both clang-tidy and
> > people)
> 
> I'm not really sure about people. This change turns some for loops from
> "iterate until we find a terminator" to "iterate n times: while
> iterating, if we find a terminator, exit the loop".

That's true of the loop itself.  But to convince myself it can't
overrun the buffer, I have to go look elsewhere to see if the
terminator is always applied.  In this case, it wasn't.  With the
explicitly bounded iteration it's immediately clear it can't overrun.

> In any case, the annoyance is minor enough, at least to me, so let's
> make clang-tidy happy by all means.
> 
> > by using ARRAY_SIZE() base bounds checking.  Treat the terminator
> > explicitly as an early exit case using 'break'.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  conf.c   | 8 ++++++--
> >  dhcp.c   | 4 +++-
> >  dhcpv6.c | 4 +++-
> >  ndp.c    | 4 +++-
> >  passt.h  | 4 ++--
> >  5 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 84ae12b2..b1fc4b9f 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1159,7 +1159,9 @@ static void conf_print(const struct ctx *c)
> >  				       buf4, sizeof(buf4)));
> >  		}
> >  
> > -		for (i = 0; !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) {
> > +		for (i = 0; i < ARRAY_SIZE(c->ip4.dns); i++) {
> > +			if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]))
> > +				break;
> >  			if (!i)
> >  				info("DNS:");
> >  			inet_ntop(AF_INET, &c->ip4.dns[i], buf4, sizeof(buf4));
> > @@ -1197,7 +1199,9 @@ static void conf_print(const struct ctx *c)
> >  			       buf6, sizeof(buf6)));
> >  
> >  dns6:
> > -		for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
> > +		for (i = 0; i < ARRAY_SIZE(c->ip6.dns); i++) {
> > +			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]))
> > +			    break;
> >  			if (!i)
> >  				info("DNS:");
> >  			inet_ntop(AF_INET6, &c->ip6.dns[i], buf6, sizeof(buf6));
> > diff --git a/dhcp.c b/dhcp.c
> > index 6b9c2e3b..1ff8cba9 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -430,7 +430,9 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
> >  	}
> >  
> >  	for (i = 0, opts[6].slen = 0;
> > -	     !c->no_dhcp_dns && !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) {
> > +	     !c->no_dhcp_dns && i < ARRAY_SIZE(c->ip4.dns); i++) {
> > +		if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]))
> > +			break;
> >  		((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i];
> >  		opts[6].slen += sizeof(uint32_t);
> >  	}
> > diff --git a/dhcpv6.c b/dhcpv6.c
> > index e4df0db5..d94be23a 100644
> > --- a/dhcpv6.c
> > +++ b/dhcpv6.c
> > @@ -425,7 +425,9 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
> >  	if (c->no_dhcp_dns)
> >  		goto search;
> >  
> > -	for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
> > +	for (i = 0; i < ARRAY_SIZE(c->ip6.dns); i++) {
> > +		if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]))
> > +			break;
> >  		if (!i) {
> >  			srv = (struct opt_dns_servers *)(buf + offset);
> >  			offset += sizeof(struct opt_hdr);
> > diff --git a/ndp.c b/ndp.c
> > index eb9e3139..1f2bcb0c 100644
> > --- a/ndp.c
> > +++ b/ndp.c
> > @@ -285,7 +285,9 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
> >  		size_t dns_s_len = 0;
> >  		int i, n;
> >  
> > -		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
> > +		for (n = 0; n < ARRAY_SIZE(c->ip6.dns); n++)
> > +			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]))
> > +				break;
> >  		if (n) {
> >  			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
> >  			*rdnss = (struct opt_rdnss) {
> > diff --git a/passt.h b/passt.h
> > index 79d01ddb..87da76d3 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -91,7 +91,7 @@ struct ip4_ctx {
> >  	struct in_addr guest_gw;
> >  	struct in_addr map_host_loopback;
> >  	struct in_addr map_guest_addr;
> > -	struct in_addr dns[MAXNS + 1];
> > +	struct in_addr dns[MAXNS];
> 
> The comment still says:
> 
>  * @dns:		DNS addresses for DHCP, zero-terminated
> 
> >  	struct in_addr dns_match;
> >  	struct in_addr our_tap_addr;
> >  
> > @@ -132,7 +132,7 @@ struct ip6_ctx {
> >  	struct in6_addr guest_gw;
> >  	struct in6_addr map_host_loopback;
> >  	struct in6_addr map_guest_addr;
> > -	struct in6_addr dns[MAXNS + 1];
> > +	struct in6_addr dns[MAXNS];
> 
> ...same here. But as this is the only remark I have on the whole
> series, I took the liberty to fix up the comment directly on merge.

Ah, thanks.

> >  	struct in6_addr dns_match;
> >  	struct in6_addr our_tap_ll;
> >  
> 
> -- 
> 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 --]