diff mbox series

resolv: Fix endless loop in __res_context_query

Message ID 20240111130118.1483134-1-stli@linux.ibm.com
State Committed
Headers
Series resolv: Fix endless loop in __res_context_query |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 fail Patch failed to apply

Commit Message

Stefan Liebler Jan. 11, 2024, 1:01 p.m. UTC 
  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.
---
 resolv/res_query.c         | 8 +++++---
 resolv/tst-resolv-qtypes.c | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Adhemerval Zanella Netto Jan. 11, 2024, 1:27 p.m. UTC | #1 
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.
Stefan Liebler Jan. 11, 2024, 3:40 p.m. UTC | #2 
On 11.01.24 14:27, Adhemerval Zanella Netto wrote:
> 
> 
> 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?
Sure. Committed with a bool.
Thanks,
Stefan
diff mbox series

Patch

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;
 
 	/* 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;
 		}
 	}
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