Re: [PATCH] resolv: Fix endless loop in __res_context_query

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 11/01/24 10:01, Stefan Liebler wrote:
> Starting with commit 40c0add7d48739f5d89ebba255c1df26629a76e2
> "resolve: Remove __res_context_query alloca usage"
> there is an endless loop in __res_context_query if
> __res_context_mkquery fails e.g. if type is invalid.  Then the
> scratch buffer is resized to MAXPACKET size and it is retried again.
> 
> Before the mentioned commit, it was retried only once and with the
> mentioned commit, there is no check and it retries in an endless loop.
> 
> This is observable with xtest resolv/tst-resolv-qtypes which times out
> after 300s.
> 
> This patch retries mkquery only once as before the mentioned commit.
> Furthermore, scratch_buffer_set_array_size is now only called with
> nelem=2 if type is T_QUERY_A_AND_AAAA (also see mentioned commit).
> The test tst-resolv-qtypes is also adjusted to verify that <func>
> is really returning with -1 in case of an invalid type.

Thanks for catching it.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  resolv/res_query.c         | 8 +++++---
>  resolv/tst-resolv-qtypes.c | 4 ++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/resolv/res_query.c b/resolv/res_query.c
> index 1b148a2a05..4bfba24c73 100644
> --- a/resolv/res_query.c
> +++ b/resolv/res_query.c
> @@ -115,7 +115,7 @@ __res_context_query (struct resolv_context *ctx, const char *name,
>  	struct __res_state *statp = ctx->resp;
>  	UHEADER *hp = (UHEADER *) answer;
>  	UHEADER *hp2;
> -	int n;
> +	int n, retried = 0;

Maybe use a bool here?

>  
>  	/* It requires 2 times QUERYSIZE for type == T_QUERY_A_AND_AAAA.  */
>  	struct scratch_buffer buf;
> @@ -182,13 +182,15 @@ __res_context_query (struct resolv_context *ctx, const char *name,
>  	    nquery1 = n;
>  	  }
>  
> -	if (__glibc_unlikely (n <= 0)) {
> +	if (__glibc_unlikely (n <= 0) && !retried) {
>  		/* Retry just in case res_nmkquery failed because of too
>  		   short buffer.  Shouldn't happen.  */
>  		if (scratch_buffer_set_array_size (&buf,
> -						   T_QUERY_A_AND_AAAA ? 2 : 1,
> +						   (type == T_QUERY_A_AND_AAAA)
> +						   ? 2 : 1,
>  						   MAXPACKET)) {
>  			query1 = buf.data;
> +			retried = 1;
>  			goto again;
>  		}
>  	}

Ok

> diff --git a/resolv/tst-resolv-qtypes.c b/resolv/tst-resolv-qtypes.c
> index 3fa566c7ea..973c4e15d3 100644
> --- a/resolv/tst-resolv-qtypes.c
> +++ b/resolv/tst-resolv-qtypes.c
> @@ -154,8 +154,8 @@ test_function (const char *fname,
>          }
>      }
>  
> -  TEST_VERIFY (func (-1, buf, sizeof (buf) == -1));
> -  TEST_VERIFY (func (65536, buf, sizeof (buf) == -1));
> +  TEST_VERIFY (func (-1, buf, sizeof (buf)) == -1);
> +  TEST_VERIFY (func (65536, buf, sizeof (buf)) == -1);
>  }
>  
>  static int

Ok.