[PATCH] resolv: Fix tests by aligning hand crafted queries

[PATCH] resolv: Fix tests by aligning hand crafted queries

[Stafford Horne]

When testing OpenRISC I get a bus error in res_send.  This is due to the
buf being cast to a (HEADER *) and trying the res_send code trying to read
different bits of the HEADER struct including 16-bit id etc.

On OpenRISC reads of 16-bits and 32-bits from structures need to be 2
byte and 4 byte aligned, respectively.

To fix this we can align the hand crafted queries.
---
 resolv/tst-resolv-binary.c  | 2 +-
 resolv/tst-resolv-trustad.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/resolv/tst-resolv-binary.c b/resolv/tst-resolv-binary.c
index 44895a1baa..f76ae057c4 100644
--- a/resolv/tst-resolv-binary.c
+++ b/resolv/tst-resolv-binary.c
@@ -52,7 +52,7 @@ do_test (void)
 
   for (int b = 0; b <= 255; ++b)
     {
-      unsigned char query[] =
+      unsigned char query[] __attribute__ ((aligned)) =
         {
           b, b,                 /* Transaction ID.  */
           1, 0,                 /* Query with RD flag.  */
diff --git a/resolv/tst-resolv-trustad.c b/resolv/tst-resolv-trustad.c
index 74ee5db735..8d6989adb4 100644
--- a/resolv/tst-resolv-trustad.c
+++ b/resolv/tst-resolv-trustad.c
@@ -93,7 +93,8 @@ do_test (void)
   /* By default, the resolver is not trusted, and the AD bit is
      cleared.  */
 
-  static const unsigned char hand_crafted_query[] =
+  static const unsigned char hand_crafted_query[]
+			     __attribute__ ((aligned)) =
     {
      10, 11,                    /* Transaction ID.  */
      1, 0x20,                   /* Query with RD, AD flags.  */
-- 
2.31.1

Re: [PATCH] resolv: Fix tests by aligning hand crafted queries

[Stafford Horne]

ping

On Tue, Jun 15, 2021 at 8:40 AM Stafford Horne <shorne@gmail.com> wrote:
>
> When testing OpenRISC I get a bus error in res_send.  This is due to the
> buf being cast to a (HEADER *) and trying the res_send code trying to read
> different bits of the HEADER struct including 16-bit id etc.
>
> On OpenRISC reads of 16-bits and 32-bits from structures need to be 2
> byte and 4 byte aligned, respectively.
>
> To fix this we can align the hand crafted queries.
> ---
>  resolv/tst-resolv-binary.c  | 2 +-
>  resolv/tst-resolv-trustad.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/resolv/tst-resolv-binary.c b/resolv/tst-resolv-binary.c
> index 44895a1baa..f76ae057c4 100644
> --- a/resolv/tst-resolv-binary.c
> +++ b/resolv/tst-resolv-binary.c
> @@ -52,7 +52,7 @@ do_test (void)
>
>    for (int b = 0; b <= 255; ++b)
>      {
> -      unsigned char query[] =
> +      unsigned char query[] __attribute__ ((aligned)) =
>          {
>            b, b,                 /* Transaction ID.  */
>            1, 0,                 /* Query with RD flag.  */
> diff --git a/resolv/tst-resolv-trustad.c b/resolv/tst-resolv-trustad.c
> index 74ee5db735..8d6989adb4 100644
> --- a/resolv/tst-resolv-trustad.c
> +++ b/resolv/tst-resolv-trustad.c
> @@ -93,7 +93,8 @@ do_test (void)
>    /* By default, the resolver is not trusted, and the AD bit is
>       cleared.  */
>
> -  static const unsigned char hand_crafted_query[] =
> +  static const unsigned char hand_crafted_query[]
> +                            __attribute__ ((aligned)) =
>      {
>       10, 11,                    /* Transaction ID.  */
>       1, 0x20,                   /* Query with RD, AD flags.  */
> --
> 2.31.1
>

Re: [PATCH] resolv: Fix tests by aligning hand crafted queries

[Adhemerval Zanella]

On 14/06/2021 20:40, Stafford Horne via Libc-alpha wrote:
> When testing OpenRISC I get a bus error in res_send.  This is due to the
> buf being cast to a (HEADER *) and trying the res_send code trying to read
> different bits of the HEADER struct including 16-bit id etc.
> 
> On OpenRISC reads of 16-bits and 32-bits from structures need to be 2
> byte and 4 byte aligned, respectively.
> 
> To fix this we can align the hand crafted queries.


But the res_send() interface does specify that buffer is an 'unsigned char',
so I think the problem is in fact send_vc() (any any other code that consume
the buffer) where the cast is in fact undefined.  I am not sure why it has
not show any issue on architecture that trap on unaligned access, may guess
is the stack buffer alignment is the same as the HEADER.

The issue is resolv code seems to abuse this...

> ---
>  resolv/tst-resolv-binary.c  | 2 +-
>  resolv/tst-resolv-trustad.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/resolv/tst-resolv-binary.c b/resolv/tst-resolv-binary.c
> index 44895a1baa..f76ae057c4 100644
> --- a/resolv/tst-resolv-binary.c
> +++ b/resolv/tst-resolv-binary.c
> @@ -52,7 +52,7 @@ do_test (void)
>  
>    for (int b = 0; b <= 255; ++b)
>      {
> -      unsigned char query[] =
> +      unsigned char query[] __attribute__ ((aligned)) =
>          {
>            b, b,                 /* Transaction ID.  */
>            1, 0,                 /* Query with RD flag.  */
> diff --git a/resolv/tst-resolv-trustad.c b/resolv/tst-resolv-trustad.c
> index 74ee5db735..8d6989adb4 100644
> --- a/resolv/tst-resolv-trustad.c
> +++ b/resolv/tst-resolv-trustad.c
> @@ -93,7 +93,8 @@ do_test (void)
>    /* By default, the resolver is not trusted, and the AD bit is
>       cleared.  */
>  
> -  static const unsigned char hand_crafted_query[] =
> +  static const unsigned char hand_crafted_query[]
> +			     __attribute__ ((aligned)) =
>      {
>       10, 11,                    /* Transaction ID.  */
>       1, 0x20,                   /* Query with RD, AD flags.  */
> 

Re: [PATCH] resolv: Fix tests by aligning hand crafted queries

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 14/06/2021 20:40, Stafford Horne via Libc-alpha wrote:
>> When testing OpenRISC I get a bus error in res_send.  This is due to the
>> buf being cast to a (HEADER *) and trying the res_send code trying to read
>> different bits of the HEADER struct including 16-bit id etc.
>> 
>> On OpenRISC reads of 16-bits and 32-bits from structures need to be 2
>> byte and 4 byte aligned, respectively.
>> 
>> To fix this we can align the hand crafted queries.
>
>
> But the res_send() interface does specify that buffer is an 'unsigned char',
> so I think the problem is in fact send_vc() (any any other code that consume
> the buffer) where the cast is in fact undefined.  I am not sure why it has
> not show any issue on architecture that trap on unaligned access, may guess
> is the stack buffer alignment is the same as the HEADER.
>
> The issue is resolv code seems to abuse this...

Right, there is a separate bug for this:

  Misaligned access in res_query.c HEADER struct
  <https://sourceware.org/bugzilla/show_bug.cgi?id=20243>

I really want to fix this, but each time I get side-tracked by other
cleanups that appear to be necessary for this. 8-(

Thanks,
Florian

Re: [PATCH] resolv: Fix tests by aligning hand crafted queries

[Stafford Horne]

On Tue, Sep 28, 2021 at 06:00:04PM +0200, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
> > On 14/06/2021 20:40, Stafford Horne via Libc-alpha wrote:
> >> When testing OpenRISC I get a bus error in res_send.  This is due to the
> >> buf being cast to a (HEADER *) and trying the res_send code trying to read
> >> different bits of the HEADER struct including 16-bit id etc.
> >> 
> >> On OpenRISC reads of 16-bits and 32-bits from structures need to be 2
> >> byte and 4 byte aligned, respectively.
> >> 
> >> To fix this we can align the hand crafted queries.
> >
> >
> > But the res_send() interface does specify that buffer is an 'unsigned char',
> > so I think the problem is in fact send_vc() (any any other code that consume
> > the buffer) where the cast is in fact undefined.  I am not sure why it has
> > not show any issue on architecture that trap on unaligned access, may guess
> > is the stack buffer alignment is the same as the HEADER.
> >
> > The issue is resolv code seems to abuse this...
> 
> Right, there is a separate bug for this:
> 
>   Misaligned access in res_query.c HEADER struct
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=20243>
> 
> I really want to fix this, but each time I get side-tracked by other
> cleanups that appear to be necessary for this. 8-(

Thanks for the comments, in the mean time does that mean my patch should not go
in as it is?

-Stafford