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

[Carlos O'Donell]

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?

-- 
Cheers,
Carlos.