Re: [PATCH] dhcpv6: Properly separate domain names in search list

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

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

On Wed, Sep 20, 2023 at 05:05:06PM +0200, Stefano Brivio wrote:
> If we go over the flattened list of search domains and just replace
> dots and zero bytes with the length of the next label to implement
> the encoding specified by section 3.1 of RFC 1035, if there are
> multiple domains in the search list, we'll also replace separators
> between two domain names with the length of the first label of the
> second domain, plus one.

That is... an impressively long sentence.  Any chance you could reword
that in shorter ones that are easier to follow ;).

> Those should remain as zero bytes to
> separate domains, though.
> 
> To distinguish between label separators and domain names separators,
> for simplicity, introduce a dot before the first label of every
> domain we copy to form the list. All dots are then replaced by label
> lengths, and separators (zero bytes) remain as they are.
> 
> As we do this, we need to make sure we don't replace the trailing
> dot, if present: that's already a separator. Skip copying it, and
> just add separators as needed.
> 
> Now that we don't copy those, though, we might end up with
> zero-length domains: skip them, as they're meaningless anyway.
> 
> And as we might skip domains, we can't use the index 'i' to check if
> we're at the beginning of the option -- use 'srch' instead.
> 
> This is very similar to how we prepare the list for NDP option 31,
> except that we don't need padding (RFC 8106, 5.2) here, and we should
> refactor this into common functions, but it probably makes sense to
> rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21)
> first.
> 
> Reported-by: Sebastian Mitterle <smitterl@redhat.com>
> Link: https://bugs.passt.top/show_bug.cgi?id=75
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  dhcpv6.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index fc42a84..58171bb 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -376,24 +376,34 @@ search:
>  		return offset;
>  
>  	for (i = 0; *c->dns_search[i].n; i++) {
> -		if (!i) {
> +		size_t name_len = strlen(c->dns_search[i].n);
> +
> +		/* We already append separators, don't duplicate if present */
> +		if (c->dns_search[i].n[name_len - 1] == '.')
> +			name_len--;
> +
> +		/* Skip root-only search domains */
> +		if (!name_len)
> +			continue;

Should we consider doing this normalisation when we build
c->dns_search, rather than here?

> +		if (!srch) {
>  			srch = (struct opt_dns_search *)(buf + offset);
>  			offset += sizeof(struct opt_hdr);
>  			srch->hdr.t = OPT_DNS_SEARCH;
>  			srch->hdr.l = 0;
>  			p = srch->list;
> -			*p = 0;
>  		}
>  
> -		p = stpcpy(p + 1, c->dns_search[i].n);
> -		*(p++) = 0;
> -		srch->hdr.l += strlen(c->dns_search[i].n) + 2;
> -		offset += strlen(c->dns_search[i].n) + 2;
> +		*p = '.';
> +		p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> +		p++;
> +		srch->hdr.l += name_len + 2;
> +		offset += name_len + 2;
>  	}
>  
>  	if (srch) {
>  		for (i = 0; i < srch->hdr.l; i++) {
> -			if (srch->list[i] == '.' || !srch->list[i]) {
> +			if (srch->list[i] == '.') {
>  				srch->list[i] = strcspn(srch->list + i + 1,
>  							".");
>  			}

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