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

[Carlos O'Donell]

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);


-- 
Cheers,
Carlos.