nss_dns: Do not replace root domain with empty string

  The purpose of the bp[0] == '.' check is unclear.  Only the root domain
starts with '.'.  An empty string is not a valid domain name, so the
subsequent res_dnok check fails.

2019-04-08  Florian Weimer  <fweimer@redhat.com>

	* resolv/nss_dns/dns-network.c (getanswer_r): Do not replace root
	domain with empty string.
	* resolv/nss_dns/dns-host.c (getanswer_r): Likewise.
  

On 4/8/19 7:18 AM, Florian Weimer wrote:
> The purpose of the bp[0] == '.' check is unclear.  Only the root domain
> starts with '.'.  An empty string is not a valid domain name, so the
> subsequent res_dnok check fails.

Was the intent to *cause* a failure if a root domain was unpacked?

Are you certain res_dnok() fails with the empty string? It looks to me
like ns_name_pton() might work, and so would printable_string(), but
the result is obviously garbage, and may fail later.

Logically the code in question doesn't make any sense from first principles,
therefore I think the deletion is warranted, and that we'll fix any fallout.

I'm OK with it for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-04-08  Florian Weimer  <fweimer@redhat.com>
> 
> 	* resolv/nss_dns/dns-network.c (getanswer_r): Do not replace root
> 	domain with empty string.
> 	* resolv/nss_dns/dns-host.c (getanswer_r): Likewise.
> 
> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index a18b8a6bf4..9c15f25f28 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -706,9 +706,6 @@ getanswer_r (struct resolv_context *ctx,
>         n = -1;
>       }
>   
> -  if (n > 0 && bp[0] == '.')
> -    bp[0] = '\0';
> -
>     if (__glibc_unlikely (n < 0))
>       {
>         *errnop = errno;
> diff --git a/resolv/nss_dns/dns-network.c b/resolv/nss_dns/dns-network.c
> index 4b81b1bfdc..21688c19b2 100644
> --- a/resolv/nss_dns/dns-network.c
> +++ b/resolv/nss_dns/dns-network.c
> @@ -345,9 +345,6 @@ getanswer_r (const querybuf *answer, int anslen, struct netent *result,
>   	  n = -1;
>   	}
>   
> -      if (n > 0 && bp[0] == '.')
> -	bp[0] = '\0';
> -
>         if (n < 0 || res_dnok (bp) == 0)
>   	break;
>         cp += n;
>
  

* Carlos O'Donell:

> On 4/8/19 7:18 AM, Florian Weimer wrote:
>> The purpose of the bp[0] == '.' check is unclear.  Only the root domain
>> starts with '.'.  An empty string is not a valid domain name, so the
>> subsequent res_dnok check fails.
>
> Was the intent to *cause* a failure if a root domain was unpacked?
>
> Are you certain res_dnok() fails with the empty string? It looks to me
> like ns_name_pton() might work, and so would printable_string(), but
> the result is obviously garbage, and may fail later.
>
> Logically the code in question doesn't make any sense from first
> principles, therefore I think the deletion is warranted, and that
> we'll fix any fallout.

Hmm, you are right, res_dnok et al. accept it, which is something we
cannot change for backwards compatibility reasons.  Odd.  There is even
a test for it.

I'm going to use this as the commit message then:

    nss_dns: Do not replace root domain with empty string
    
    The purpose of the bp[0] == '.' check is unclear.  Only the root domain
    starts with '.'.  The empty string is accepted as a domain name in many
    places, denoting the root, but using it implicitly is confusing.

Still okay?

Thansk,
Florian
  

On 4/8/19 11:07 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 4/8/19 7:18 AM, Florian Weimer wrote:
>>> The purpose of the bp[0] == '.' check is unclear.  Only the root domain
>>> starts with '.'.  An empty string is not a valid domain name, so the
>>> subsequent res_dnok check fails.
>>
>> Was the intent to *cause* a failure if a root domain was unpacked?
>>
>> Are you certain res_dnok() fails with the empty string? It looks to me
>> like ns_name_pton() might work, and so would printable_string(), but
>> the result is obviously garbage, and may fail later.
>>
>> Logically the code in question doesn't make any sense from first
>> principles, therefore I think the deletion is warranted, and that
>> we'll fix any fallout.
> 
> Hmm, you are right, res_dnok et al. accept it, which is something we
> cannot change for backwards compatibility reasons.  Odd.  There is even
> a test for it.
> 
> I'm going to use this as the commit message then:
> 
>      nss_dns: Do not replace root domain with empty string
>      
>      The purpose of the bp[0] == '.' check is unclear.  Only the root domain
>      starts with '.'.  The empty string is accepted as a domain name in many
>      places, denoting the root, but using it implicitly is confusing.
> 
> Still okay?

Yes. I'm OK with the adjusted commit message.