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

[Carlos O'Donell]

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?

-- 
Cheers,
Carlos.