| Message ID | 20240111130118.1483134-1-stli@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | resolv: Fix endless loop in __res_context_query | expand |
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.
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 --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