[RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Ludwig Rydberg]

Dear maintainers,

When running the resolv test suite on sparc32 (leon, linux 5.10) I saw
that several tests failed (due to SIGBUS):

FAIL: resolv/tst-bug18665
FAIL: resolv/tst-bug18665-tcp
FAIL: resolv/tst-resolv-basic
FAIL: resolv/tst-resolv-edns
FAIL: resolv/tst-resolv-invalid-cname
FAIL: resolv/tst-resolv-res_init-multi
FAIL: resolv/tst-resolv-search

Test output:
Didn't expect signal from child: got `Bus error'

When analyzing the root cause it was found that the tests started to
fail after the following patch was submitted:

40c0add7d487 ("resolve: Remove __res_context_query alloca usage")

The reason the test cases crash is because of unaligned access.
By re-introducing the alignment handling the test suite pass on sparc32.

I noticed when reading the review comments about 40c0add7d487 that it was
suggested to remove the alignement code.

So, can the alignment code be re-introduced or should this be handled in
another way? 

Best regards,
// Ludwig

Ludwig Rydberg (1):
  resolv: Align buf before second query for A_AAA in __res_context_query

 resolv/res_query.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)


base-commit: 7b12776584c51dbecb1033e107f6b9f45de47a1b
-- 
2.35.3

[RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Ludwig Rydberg]

Re-introduces alignment handling that was removed by patch 40c0add7d487
("resolve: Remove __res_context_query alloca usage")

Verfied by successfully running the resolv test suite on sparc32.

Signed-off-by: Ludwig Rydberg <ludwig.rydberg@gaisler.com>
---
 resolv/res_query.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/resolv/res_query.c b/resolv/res_query.c
index 1b148a2a05b8..d065814b38e8 100644
--- a/resolv/res_query.c
+++ b/resolv/res_query.c
@@ -147,9 +147,19 @@ __res_context_query (struct resolv_context *ctx, const char *name,
 		  }
 
 		nquery1 = n;
-		query2 = buf.data + n;
+		/* Align the buffer.  */
+		int npad = ((nquery1 + __alignof__ (HEADER) - 1)
+			    & ~(__alignof__ (HEADER) - 1)) - nquery1;
+		if (n > buf.length - npad)
+		  {
+		    n = -1;
+		    goto unspec_nomem;
+		  }
+
+		int nused = n + npad;
+		query2 = buf.data + nused;
 		n = __res_context_mkquery (ctx, QUERY, name, class, T_AAAA,
-					   NULL, query2, buf.length - n);
+					   NULL, query2, buf.length - nused);
 		if (n > 0
 		    && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0)
 		  /* Use RESOLV_EDNS_BUFFER_SIZE because the receive
-- 
2.35.3

Re: [RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Florian Weimer]

* Ludwig Rydberg:

> So, can the alignment code be re-introduced or should this be handled
> in another way?

The res_* functions do not require aligned buffers as part of the API,
so I think if they have such an implicit dependency, in needs to be
fixed in their implementation.

Thanks,
Florian

Re: [RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Ludwig Rydberg]

Thanks for your quick reply.

Ok, just a question (to avoid misinterpretation), when you say "... it 
needs to be fixed in their implementation", what are you referring to?

Best regards,
// Ludwig

On 2023-11-30 12:53, Florian Weimer wrote:
> * Ludwig Rydberg:
> 
>> So, can the alignment code be re-introduced or should this be handled
>> in another way?
> 
> The res_* functions do not require aligned buffers as part of the API,
> so I think if they have such an implicit dependency, in needs to be
> fixed in their implementation.
> 
> Thanks,
> Florian
> 

Re: [RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Florian Weimer]

* Ludwig Rydberg:

> Thanks for your quick reply.
>
> Ok, just a question (to avoid misinterpretation), when you say "... it
> needs to be fixed in their implementation", what are you referring to?

I was a bit vague because you didn't say were it crashed.  If it's
res_mkquery, I suggest to fix it there.

Thanks,
Florian

Re: [RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Ludwig Rydberg]

On 2023-11-30 14:14, Florian Weimer wrote:
> 
> I was a bit vague because you didn't say were it crashed.  If it's
> res_mkquery, I suggest to fix it there.

True, I could have given some more details. It crash in 
res_mkquery:__res_context_mkquery for the second query when using the 
unaligned buffer (from the first query).

Thanks, I will take a look.

Best regards,
// Ludwig

Re: [RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Ludwig Rydberg]

Hi Florian,

I took some time to investigate the issue.

As I understand it, the __res_context_query might need to create two 
queries in case the type is T_QUERY_A_AND_AAAA.
In this case, a single buffer with enough room to hold two queries is used.

The first query fills up n bytes of the buffer and the second query is 
added just after:
buf: |<---query1-->|<---query2--->|

Then since the length of query1 varies, query2 might start on an 
unaligned address.
And that leads to SIGBUS on sparc32+linux in __res_context_mkquery when 
the unaligned query2-buf is casted to a HEADER and accessed.

Initially I had a look at __res_context_mkquery (as you suggested) but 
couldn't really see how this could be handled there efficiently.
There are also other functions (__res_nopt, __res_handle_no_aaaa, 
__res_context_send, ...) that use the query later on which would then 
led to the same crash.

It could be solved by using two separate buffers for the queries but I 
don't think that is the right way.
I understand that the api does not require aligned buffers but since 
this is part of an optimization (same buffer for both queries) perhaps 
it could be fine?

Any thoughts or ideas on how to proceed?

Best regards,
// Ludwig

On 2023-11-30 16:11, Ludwig Rydberg wrote:
> On 2023-11-30 14:14, Florian Weimer wrote:
>>
>> I was a bit vague because you didn't say were it crashed.  If it's
>> res_mkquery, I suggest to fix it there.
> 
> True, I could have given some more details. It crash in 
> res_mkquery:__res_context_mkquery for the second query when using the 
> unaligned buffer (from the first query).
> 
> Thanks, I will take a look.
> 
> Best regards,
> // Ludwig

Re: [RFC] resolv: Align buf before second query for A_AAA in __res_context_query

[Florian Weimer]

* Ludwig Rydberg:

> Initially I had a look at __res_context_mkquery (as you suggested) but
> couldn't really see how this could be handled there efficiently.
> There are also other functions (__res_nopt, __res_handle_no_aaaa,
> __res_context_send, ...) that use the query later on which would then
> led to the same crash.

They all probably needed to use UHEADER * instead of HEADER *.  See this
commit for reference:

commit c8fa383f4cec9cf1c0cc8ec97903c09af10286f4
Author: John David Anglin <danglin@gcc.gnu.org>
Date:   Wed Sep 13 11:04:41 2023 +0000

    resolv: Fix some unaligned accesses in resolver [BZ #30750]
    
    Signed-off-by: John David Anglin <dave.anglin@bell.net>

Thanks,
Florian