From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202512 header.b=fem//OuK; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id E079D5A0271 for ; Sun, 11 Jan 2026 03:32:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202512; t=1768098757; bh=1hgc1Ckh/Kcc+LZxDt1xnNIOzqKJxgrK/FGh71Gqp/g=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fem//OuKLGR6N9zQnC5RKSsPMYYpbql6xqEOwX9tHMaySHv+lh7kPNX14z65QiFxT B10LU6mBIrMpxDgqk4DcvOSryZlIAD7RrrVQ/83xn39dpsIMlVEpjpyDVznOgL98g2 WhOW1ljwOI2K0gnfiylmApaHyocs9sJ/xB7FILULp1UZwEhjDxKV4EsEPvXNz1pOfk uYOiOCFdP1Z7R58uxo0MVE8+75Ag8MSEqmERAKVnUeNgf1Di5pmHqyHhNzyrtqw6bV zOyHTWV1nB7yKC4tTAw99lUTqJqKnV/74Qsrca6NnVI7yQdC6OwPLfSALrVkOO1FI5 b02yKBJ57JaxQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4dpfg91VpMz4wM3; Sun, 11 Jan 2026 13:32:37 +1100 (AEDT) Date: Sun, 11 Jan 2026 13:32:16 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 3/5] treewide: Don't rely on terminator records in ip[46].dns arrays Message-ID: References: <20260107014606.1513722-1-david@gibson.dropbear.id.au> <20260107014606.1513722-4-david@gibson.dropbear.id.au> <20260111003300.077bfacf@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="f5yFJuI8ZYZWn49p" Content-Disposition: inline In-Reply-To: <20260111003300.077bfacf@elisabeth> Message-ID-Hash: K2HPII6MBVVHDOFGYFHFLY7T4REZDBNN X-Message-ID-Hash: K2HPII6MBVVHDOFGYFHFLY7T4REZDBNN X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --f5yFJuI8ZYZWn49p Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Jan 11, 2026 at 12:33:00AM +0100, Stefano Brivio wrote: > On Wed, 7 Jan 2026 12:46:04 +1100 > David Gibson wrote: >=20 > > 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 arr= ay, > > 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. > >=20 > > 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 >=3D 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. > >=20 > > Fix the bug, and make it easier to reason about (for both clang-tidy and > > people) >=20 > 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. >=20 > > by using ARRAY_SIZE() base bounds checking. Treat the terminator > > explicitly as an early exit case using 'break'. > >=20 > > Signed-off-by: David Gibson > > Reviewed-by: Laurent Vivier > > --- > > conf.c | 8 ++++++-- > > dhcp.c | 4 +++- > > dhcpv6.c | 4 +++- > > ndp.c | 4 +++- > > passt.h | 4 ++-- > > 5 files changed, 17 insertions(+), 7 deletions(-) > >=20 > > 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))); > > } > > =20 > > - for (i =3D 0; !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) { > > + for (i =3D 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))); > > =20 > > dns6: > > - for (i =3D 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) { > > + for (i =3D 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) > > } > > =20 > > for (i =3D 0, opts[6].slen =3D 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] =3D c->ip4.dns[i]; > > opts[6].slen +=3D 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; > > =20 > > - for (i =3D 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) { > > + for (i =3D 0; i < ARRAY_SIZE(c->ip6.dns); i++) { > > + if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i])) > > + break; > > if (!i) { > > srv =3D (struct opt_dns_servers *)(buf + offset); > > offset +=3D 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 struc= t in6_addr *dst) > > size_t dns_s_len =3D 0; > > int i, n; > > =20 > > - for (n =3D 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++); > > + for (n =3D 0; n < ARRAY_SIZE(c->ip6.dns); n++) > > + if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n])) > > + break; > > if (n) { > > struct opt_rdnss *rdnss =3D (struct opt_rdnss *)ptr; > > *rdnss =3D (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]; >=20 > The comment still says: >=20 > * @dns: DNS addresses for DHCP, zero-terminated >=20 > > struct in_addr dns_match; > > struct in_addr our_tap_addr; > > =20 > > @@ -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]; >=20 > ...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; > > =20 >=20 > --=20 > Stefano >=20 --=20 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 --f5yFJuI8ZYZWn49p Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmljC6EACgkQzQJF27ox 2GeI5w/+PsTSYUVe96YXYq3pOJ5sJa4O+YB5bQCNzDeeabWkZQXUsjd/Z+GPiACO TnIs2gB5YzFmCXok1rg3Sw70bXAbWtORsAX/6KkLXBbjoG7TaomYJhcIdaSowvgH VujZmyzDftegCkU/PT3iBw9gNvimmczWhc5X53TZWreYoapAAcS/3epr6VWYkyIc 7bINZdk53LPPdL9LAewudjHIM3V+UJl24Y05Ji3ewzQv74HgLrwR+/NhrSvPp6t6 00t4ixSe8p3C56johu3k/o/vo7v/i8Tkz2oBhzw7XCYrt06WJPuNAybJRUvYUjBS xWV5GrHdZl/7VnXNi2XsjZQVzQEmIdncJojg6+nm5OX4INRF4KwF9ZwC1CpUUruS gVmA7QfSYeNvE/YONfilzaTOJpPhIAxQsUhfo4hhRJxiIXd0MMjzmKmZDsoITeGV YOGSh8X2tF+lweTRYQ2j+Ak+b3KCRO/ChI2S8kWfjSr+qJSX1WbXSTOBdUI1RmF3 F3SazVpibbG6j8Gdji81sW+jqNmGA1qPgXnBabjNowj0AZfshtbU8z/Y08jFGdZl e3y82QoCT+CIAvWys95U31tl/5b5UXLZnA7/q75BgXOb5GNW2pN4tXHRhlFMrNgu pViq36SeRM1rd1pmfY13CfEG6nOBWy43ndj9nQNw/+fSUtQxm68= =SiLY -----END PGP SIGNATURE----- --f5yFJuI8ZYZWn49p--