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

[Stefano Brivio <sbrivio@redhat.com>]

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

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.

>  	struct in6_addr dns_match;
>  	struct in6_addr our_tap_ll;
>  

-- 
Stefano