[2/2] resolv: Avoid duplicate query if search list contains '.' (bug 33804)

  From: Carlos Peón Costa <carlospeon@gmail.com>

Co-authored-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Florian Weimer <fweimer@redhat.com>
---
Tested on x864_64-linux-gnu.

 resolv/res_query.c            | 11 +++++++----
 resolv/tst-resolv-no-search.c |  9 +++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
  

On 2/16/26 8:49 AM, Florian Weimer wrote:
> From: Carlos Peón Costa <carlospeon@gmail.com>

Does the static volatile needs to use atomics between the main thread
and the server thread? I don't see any way to reliably guarantee you
always see the server thread stores.

Second is a note about the comment adjustment ahead of the changes you
make to skip the second query.
  
> Co-authored-by: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Florian Weimer <fweimer@redhat.com>
> ---
> Tested on x864_64-linux-gnu.
> 
>   resolv/res_query.c            | 11 +++++++----
>   resolv/tst-resolv-no-search.c |  9 +++++++++
>   2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/resolv/res_query.c b/resolv/res_query.c
> index 039c25a3c3..30ace9d06d 100644
> --- a/resolv/res_query.c
> +++ b/resolv/res_query.c
> @@ -354,7 +354,7 @@ __res_context_search (struct resolv_context *ctx,
>   	char tmp[NS_MAXDNAME];
>   	u_int dots;
>   	int trailing_dot, ret, saved_herrno;
> -	int got_nodata = 0, got_servfail = 0, root_on_list = 0;
> +	int got_nodata = 0, got_servfail = 0;

OK. Drop root_on_list var.

>   	int tried_as_is = 0;
>   	int searched = 0;
>   
> @@ -433,8 +433,11 @@ __res_context_search (struct resolv_context *ctx,
>   			   domain.  */

There is a comment here just before the code:

425                         /* __res_context_querydoman concatenates name
426                            with dname with a "." in between.  If we
427                            pass it in dname the "." we got from the
428                            configured default search path, we'll end
429                            up with "name..", which won't resolve.
430                            OTOH, passing it "" will result in "name.",
431                            which has the intended effect for both
432                            possible representations of the root
433                            domain.  */

Should we update this comment to mention the skipping behaviour we just added?

>   			if (dname[0] == '.')
>   				dname++;
> -			if (dname[0] == '\0')
> -				root_on_list++;
> +			if (dname[0] == '\0') {
> +				if (tried_as_is)
> +					continue;

OK. If we tried 'as is' earlier, having set tried_as_is to 1, then we continue
here skipping the __res_context_querydomain call below which would have been
a second duplicate query.

> +				tried_as_is++;
> +			}
>   
>   			ret = __res_context_querydomain
>   			  (ctx, name, dname, class, type,
> @@ -506,7 +509,7 @@ __res_context_search (struct resolv_context *ctx,
>   	 * unless RES_NOTLDQUERY is set and there were no dots.
>   	 */
>   	if ((dots || !searched || (statp->options & RES_NOTLDQUERY) == 0)
> -	    && !(tried_as_is || root_on_list)) {
> +	    && !tried_as_is) {

OK. Again, this would be a third retry, but we use tried_as_is to avoid it.

>   		ret = __res_context_querydomain
>   		  (ctx, name, NULL, class, type,
>   		   answer, anslen, answerp, answerp2, nanswerp2,
> diff --git a/resolv/tst-resolv-no-search.c b/resolv/tst-resolv-no-search.c
> index 29701d4772..cde2812638 100644
> --- a/resolv/tst-resolv-no-search.c
> +++ b/resolv/tst-resolv-no-search.c
> @@ -27,6 +27,8 @@
>   #include <support/resolv_test.h>
>   #include <support/support.h>
>   
> +static volatile int query_count;

The test thread makes a resolver call, which is handled by the
resolver server thread, and technically only one is executing at the
same time. Thus this is not UB, but without atomics it's possible you
see a stale value, even with volatile? Volatile just instructs the
compiler not to optimize away the TEST_COMPARE load, but another CPU
running the server thread need not have flushed any writes. I think
a better solution is atomic_fetch_add_release() in response(), with
atomic_store_release() in check_h to set to 0, and atomic_load_acquire()
in TEST_COMPARE to get a value from the thread. I don't see any other locks
that would force the values to be synchronized (other than obj->lock used
for termination_requested).

> +
>   /* Check that plain res_init loads the configuration as expected.  */
>   static void
>   test_res_init (void *ignored)
> @@ -41,6 +43,7 @@ response (const struct resolv_response_context *ctx,
>             struct resolv_response_builder *b,
>             const char *qname, uint16_t qclass, uint16_t qtype)
>   {
> +  ++query_count;

OK. First response.

>     TEST_VERIFY_EXIT (qclass == C_IN);
>     TEST_COMPARE (ctx->server_index, 0);
>   
> @@ -82,12 +85,16 @@ check_h (const char *name, int family, const char *expected)
>     if (family == AF_INET)
>       {
>         char *query = xasprintf ("gethostbyname (\"%s\")", name);
> +      query_count = 0;
>         check_hostent (query, gethostbyname (name), expected);
> +      TEST_COMPARE (query_count, 1);

OK. Make sure we only did one query.

>         free (query);
>       }
>     {
>       char *query = xasprintf ("gethostbyname2 (\"%s\", %d)", name, family);
> +    query_count = 0;
>       check_hostent (query, gethostbyname2 (name, family), expected);
> +    TEST_COMPARE (query_count, 1);

OK. Likewise.

>       free (query);
>     }
>   }
> @@ -98,8 +105,10 @@ check_ai (const char *name, int family, const char *expected)
>     struct addrinfo hints = { .ai_family = family, .ai_socktype = SOCK_STREAM, };
>     struct addrinfo *ai;
>     char *query = xasprintf ("%s:80 [%d]", name, hints.ai_family);
> +  query_count = 0;
>     int ret = getaddrinfo (name, "80", &hints, &ai);
>     check_addrinfo (query, ai, ret, expected);
> +  TEST_COMPARE (query_count, family == AF_UNSPEC ? 2 : 1);

OK. Depends on family, could be 1 or 2 (but not more).

>     if (ret == 0)
>       freeaddrinfo (ai);
>     free (query);
  

* Carlos O'Donell:

>>   	int tried_as_is = 0;
>>   	int searched = 0;
>>   @@ -433,8 +433,11 @@ __res_context_search (struct resolv_context
>> *ctx,
>>   			   domain.  */
>
> There is a comment here just before the code:
>
> 425                         /* __res_context_querydoman concatenates name
> 426                            with dname with a "." in between.  If we
> 427                            pass it in dname the "." we got from the
> 428                            configured default search path, we'll end
> 429                            up with "name..", which won't resolve.
> 430                            OTOH, passing it "" will result in "name.",
> 431                            which has the intended effect for both
> 432                            possible representations of the root
> 433                            domain.  */
>
> Should we update this comment to mention the skipping behaviour we just added?

I think behavior as documented in the comment has not changed.  The
skipping behavior of duplicate queries (the tried-as-is behavior) was
already there, just not correctly applied.

>> diff --git a/resolv/tst-resolv-no-search.c b/resolv/tst-resolv-no-search.c
>> index 29701d4772..cde2812638 100644
>> --- a/resolv/tst-resolv-no-search.c
>> +++ b/resolv/tst-resolv-no-search.c
>> @@ -27,6 +27,8 @@
>>   #include <support/resolv_test.h>
>>   #include <support/support.h>
>>   +static volatile int query_count;
>
> The test thread makes a resolver call, which is handled by the
> resolver server thread, and technically only one is executing at the
> same time. Thus this is not UB, but without atomics it's possible you
> see a stale value, even with volatile? Volatile just instructs the
> compiler not to optimize away the TEST_COMPARE load, but another CPU
> running the server thread need not have flushed any writes. I think
> a better solution is atomic_fetch_add_release() in response(), with
> atomic_store_release() in check_h to set to 0, and atomic_load_acquire()
> in TEST_COMPARE to get a value from the thread. I don't see any other locks
> that would force the values to be synchronized (other than obj->lock used
> for termination_requested).

I thought it was required because _THROW was used in NSS function
declarations (which implies attribute leaf).  But the tested functions
do not actually have this, so I can drop the volatile.  (But similar
constructs are used in other resolver tests, maybe also unnecessarily.)

Thanks,
Florian
  

On 3/2/26 9:09 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>>    	int tried_as_is = 0;
>>>    	int searched = 0;
>>>    @@ -433,8 +433,11 @@ __res_context_search (struct resolv_context
>>> *ctx,
>>>    			   domain.  */
>>
>> There is a comment here just before the code:
>>
>> 425                         /* __res_context_querydoman concatenates name
>> 426                            with dname with a "." in between.  If we
>> 427                            pass it in dname the "." we got from the
>> 428                            configured default search path, we'll end
>> 429                            up with "name..", which won't resolve.
>> 430                            OTOH, passing it "" will result in "name.",
>> 431                            which has the intended effect for both
>> 432                            possible representations of the root
>> 433                            domain.  */
>>
>> Should we update this comment to mention the skipping behaviour we just added?
> 
> I think behavior as documented in the comment has not changed.  The
> skipping behavior of duplicate queries (the tried-as-is behavior) was
> already there, just not correctly applied.

Sounds good then, and so no change required.

>>> diff --git a/resolv/tst-resolv-no-search.c b/resolv/tst-resolv-no-search.c
>>> index 29701d4772..cde2812638 100644
>>> --- a/resolv/tst-resolv-no-search.c
>>> +++ b/resolv/tst-resolv-no-search.c
>>> @@ -27,6 +27,8 @@
>>>    #include <support/resolv_test.h>
>>>    #include <support/support.h>
>>>    +static volatile int query_count;
>>
>> The test thread makes a resolver call, which is handled by the
>> resolver server thread, and technically only one is executing at the
>> same time. Thus this is not UB, but without atomics it's possible you
>> see a stale value, even with volatile? Volatile just instructs the
>> compiler not to optimize away the TEST_COMPARE load, but another CPU
>> running the server thread need not have flushed any writes. I think
>> a better solution is atomic_fetch_add_release() in response(), with
>> atomic_store_release() in check_h to set to 0, and atomic_load_acquire()
>> in TEST_COMPARE to get a value from the thread. I don't see any other locks
>> that would force the values to be synchronized (other than obj->lock used
>> for termination_requested).
> 
> I thought it was required because _THROW was used in NSS function
> declarations (which implies attribute leaf).  But the tested functions
> do not actually have this, so I can drop the volatile.  (But similar
> constructs are used in other resolver tests, maybe also unnecessarily.)

You are correct that __THROW implies __attribute__ ((__leaf__)) within the
NSS functions, which tells the compiler they do not call back into the TUs
definitions. They do call back though via response()? So you marked them
volatile for that purpose? I had not considered this aspect of the
implementation when I reviewed this, so I think you probably have to keep
the volatile.

Yes, regarding __THROW, gethostbyname, gethostbyname2, and getaddrinfo are
all cancellation points and so are not marked __THROW, so they *can* call
back into the caller's TU and modify data... that means volatile is not
strictly required.

Do we still consider these two distinct issues?

  * Remove use of volatile because none of the called functions are __THROW?
  * Addition of atomics to create synchronizes with behaviour to observe results?
  

* Carlos O'Donell:

>> I thought it was required because _THROW was used in NSS function
>> declarations (which implies attribute leaf).  But the tested functions
>> do not actually have this, so I can drop the volatile.  (But similar
>> constructs are used in other resolver tests, maybe also unnecessarily.)
>
> You are correct that __THROW implies __attribute__ ((__leaf__)) within the
> NSS functions, which tells the compiler they do not call back into the TUs
> definitions. They do call back though via response()? So you marked them
> volatile for that purpose? I had not considered this aspect of the
> implementation when I reviewed this, so I think you probably have to keep
> the volatile.
>
> Yes, regarding __THROW, gethostbyname, gethostbyname2, and getaddrinfo are
> all cancellation points and so are not marked __THROW, so they *can* call
> back into the caller's TU and modify data... that means volatile is not
> strictly required.
>
> Do we still consider these two distinct issues?
>
>  * Remove use of volatile because none of the called functions are __THROW?
>  * Addition of atomics to create synchronizes with behaviour to observe results?

Atomics are not needed because the DNS packet exchange provides
synchronization.  The DNS interaction completes before the getaddrinfo
etc. calls return to the main program.

Thanks,
Florian
  

On 3/2/26 12:57 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> I thought it was required because _THROW was used in NSS function
>>> declarations (which implies attribute leaf).  But the tested functions
>>> do not actually have this, so I can drop the volatile.  (But similar
>>> constructs are used in other resolver tests, maybe also unnecessarily.)
>>
>> You are correct that __THROW implies __attribute__ ((__leaf__)) within the
>> NSS functions, which tells the compiler they do not call back into the TUs
>> definitions. They do call back though via response()? So you marked them
>> volatile for that purpose? I had not considered this aspect of the
>> implementation when I reviewed this, so I think you probably have to keep
>> the volatile.
>>
>> Yes, regarding __THROW, gethostbyname, gethostbyname2, and getaddrinfo are
>> all cancellation points and so are not marked __THROW, so they *can* call
>> back into the caller's TU and modify data... that means volatile is not
>> strictly required.
>>
>> Do we still consider these two distinct issues?
>>
>>   * Remove use of volatile because none of the called functions are __THROW?
>>   * Addition of atomics to create synchronizes with behaviour to observe results?
> 
> Atomics are not needed because the DNS packet exchange provides
> synchronization.  The DNS interaction completes before the getaddrinfo
> etc. calls return to the main program.
Within the server thread there is a call to response_callback, and during
the execution of that function the global variable is altered along with
the response buffer.

I concur that the writev() from the server thread is sufficient to
synchronize the contents of the buffer, but I don't see a strong guarantee
that the global you just added has such a guarantee?

I think atomics are not needed for the contents of the results because
both writev() and sentdo() ensure the contents are visible to the receiving
thread, but I see no such argument for the memory increment.

My position continues to be that we're just getting lucky and that eventually
we'll see failures in this test from first principles, but it depends on
the hardware.

You've addressed my concerns here and we've talked it out, and the the change
is an incremental improvement. I'm willing to see this committed.

Do we need a v2 with volatile removed because none of these functions are
__THROW?
  

* Carlos O'Donell:

> On 3/2/26 12:57 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>>> I thought it was required because _THROW was used in NSS function
>>>> declarations (which implies attribute leaf).  But the tested functions
>>>> do not actually have this, so I can drop the volatile.  (But similar
>>>> constructs are used in other resolver tests, maybe also unnecessarily.)
>>>
>>> You are correct that __THROW implies __attribute__ ((__leaf__)) within the
>>> NSS functions, which tells the compiler they do not call back into the TUs
>>> definitions. They do call back though via response()? So you marked them
>>> volatile for that purpose? I had not considered this aspect of the
>>> implementation when I reviewed this, so I think you probably have to keep
>>> the volatile.
>>>
>>> Yes, regarding __THROW, gethostbyname, gethostbyname2, and getaddrinfo are
>>> all cancellation points and so are not marked __THROW, so they *can* call
>>> back into the caller's TU and modify data... that means volatile is not
>>> strictly required.
>>>
>>> Do we still consider these two distinct issues?
>>>
>>>   * Remove use of volatile because none of the called functions are __THROW?
>>>   * Addition of atomics to create synchronizes with behaviour to observe results?
>> Atomics are not needed because the DNS packet exchange provides
>> synchronization.  The DNS interaction completes before the getaddrinfo
>> etc. calls return to the main program.
> Within the server thread there is a call to response_callback, and during
> the execution of that function the global variable is altered along with
> the response buffer.
>
> I concur that the writev() from the server thread is sufficient to
> synchronize the contents of the buffer, but I don't see a strong guarantee
> that the global you just added has such a guarantee?

The test already fails if the query goes to a secondary server:

+  ++query_count;
   TEST_VERIFY_EXIT (qclass == C_IN);
   TEST_COMPARE (ctx->server_index, 0);

Individually, each UDP server is single-threaded.  The test does not
trigger TCP fallback, either, so we won't get parallelism from that,
either.

Thanks,
Florian
  

On 3/3/26 8:37 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 3/2/26 12:57 PM, Florian Weimer wrote:
>>> * Carlos O'Donell:
>>>
>>>>> I thought it was required because _THROW was used in NSS function
>>>>> declarations (which implies attribute leaf).  But the tested functions
>>>>> do not actually have this, so I can drop the volatile.  (But similar
>>>>> constructs are used in other resolver tests, maybe also unnecessarily.)
>>>>
>>>> You are correct that __THROW implies __attribute__ ((__leaf__)) within the
>>>> NSS functions, which tells the compiler they do not call back into the TUs
>>>> definitions. They do call back though via response()? So you marked them
>>>> volatile for that purpose? I had not considered this aspect of the
>>>> implementation when I reviewed this, so I think you probably have to keep
>>>> the volatile.
>>>>
>>>> Yes, regarding __THROW, gethostbyname, gethostbyname2, and getaddrinfo are
>>>> all cancellation points and so are not marked __THROW, so they *can* call
>>>> back into the caller's TU and modify data... that means volatile is not
>>>> strictly required.
>>>>
>>>> Do we still consider these two distinct issues?
>>>>
>>>>    * Remove use of volatile because none of the called functions are __THROW?
>>>>    * Addition of atomics to create synchronizes with behaviour to observe results?
>>> Atomics are not needed because the DNS packet exchange provides
>>> synchronization.  The DNS interaction completes before the getaddrinfo
>>> etc. calls return to the main program.
>> Within the server thread there is a call to response_callback, and during
>> the execution of that function the global variable is altered along with
>> the response buffer.
>>
>> I concur that the writev() from the server thread is sufficient to
>> synchronize the contents of the buffer, but I don't see a strong guarantee
>> that the global you just added has such a guarantee?
> 
> The test already fails if the query goes to a secondary server:
> 
> +  ++query_count;
>     TEST_VERIFY_EXIT (qclass == C_IN);
>     TEST_COMPARE (ctx->server_index, 0);
> 
> Individually, each UDP server is single-threaded.  The test does not
> trigger TCP fallback, either, so we won't get parallelism from that,
> either.

Correct, and my point is not about two server threads, which if we had
them in this case would be UB (two threads reading and writing to the
same memory). In this case the response callback has sufficient state
to know it won't be called in parallel and so doesn't need atomics for
this specific reason.

My comment is about memory synchronization between the test thread,
and the server thread.

If the test thread and the server thread run on distinct cores, there
is no synchronizes-with for the memory used by query_count, and as such
the read on the test thread may see an old value.

To see such a failure requires a large enough memory buffer on each core
and for the threads to have been scheduled on distinct cores that do not
share cache hierarchies.

In the case of the single threaded polling udp server thread, it uses
sendto() to send the data, and so the buffers are synchronized, but as
noted I don't see a guarantee that any other memory is synchronized.

Does that clarify my design concern?