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

[Florian Weimer]

* 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