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

[Laurent Vivier <lvivier@redhat.com>]

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

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