Re: [PATCH 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: 5043 bytes --]

On Tue, Jan 06, 2026 at 02:53:24PM +0100, Laurent Vivier wrote:
> On 1/5/26 08:53, David Gibson 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) 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>
> > ---
> >   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..ceb9aa55 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;
> 
> It should be "\t" rather than "    ".

Oops, not sure how that happened.  Fixed.

> 
> Otherwise:
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> >   			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];
> >   	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];
> >   	struct in6_addr dns_match;
> >   	struct in6_addr our_tap_ll;
> 

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