Busybox 1.16.x dnsd alignment problems

[Lars Reemts]

> Are you saying that here:

> # define move_from_unaligned16(v, u16p) (memcpy(&(v), (u16p), 2))

> memcpy may be opportunistic and if u16p has type uin16_t,
> memcpy will be optimized to assignment?

Definitely yes.

> Please find a way which works for your arch and compiler,
> and let me know. Parhaps this?

> # define move_from_unaligned16(v, u16p) (memcpy(&(v), (void*)(u16p), 2))

This also was my first idea, but it doesn't work. Seems like gcc tries
to be super clever, ignoring the cast and using the original pointer
type instead.
Maybe something like

# define move_from_unaligned16(v, u16p) {void*
src=(void*)(u16p);memcpy(&(v), src, 2);}

will do the trick. I will try it tomorrow.


Denys Vlasenko schrieb:
> On Wed, Apr 14, 2010 at 6:02 AM, Lars Reemts <lars at reemts.de> wrote:
>   
>> Hello,
>>
>> I'm using busybox on a AT91SAM9261 (arm920t) processor with gcc 4.3.4.
>> Yesterday I ran into some problems using dnsd. DNS queries from a
>> windows host were not properly decoded and hence not answered. Digging
>> into the code the problem turned out to be two different problems, both
>> related to halfword alignment.
>>
>> The first one (in dnsd_main()) is related to the alignment of buffer
>> where the incoming network packets are stored. It was declared and
>> allocated as uint8_t buf[] in dnsd.c:462. This tells the compiler that
>> the buffer is containing byte data and gives it the freedom to allocate
>> the buffer at any address. In my setup it starts at an odd address. When
>> processing an incoming packet later on in process_packet() the buffer is
>> accessed as containing 2 byte data, which leads to alignment problems.
>> Allocating the buffer as uint16_t buf[] solves the problem for me.
>>
>> The second one (in process_packet()) was particularly hard to track
>> down. It boils down to move_from_unaligned16(), which is #defined as 2
>> byte memcpy() in platform.h not working correctly for 16 bit source
>> pointer types. For these the compiler assumes the pointer to already be
>> aligned and replaces the 2 byte memcpy() by a half word load and a half
>> word store assembler instruction. The solution is to ensure the source
>> pointer is pointing to a single byte data type.
>>     
>
> Are you saying that here:
>
> # define move_from_unaligned16(v, u16p) (memcpy(&(v), (u16p), 2))
>
> memcpy may be opportunistic and if u16p has type uin16_t,
> memcpy will be optimized to assignment? If so, this
> should be prevented from happening. We WANT to be damn sure
> any address, however badly unaligned, will work in this macro.
>
> Please find a way which works for your arch and compiler,
> and let me know. Parhaps this?
>
> # define move_from_unaligned16(v, u16p) (memcpy(&(v), (void*)(u16p), 2))
>
>   
>> Please have a look at the attached patch which solves both problems. I'm
>> not sure whether the solution for the second problem should be done in
>> platform.h.
>>     
>
> +       /* allocate the buffer as uint16_t[] to make sure it is
> aligned at a uint16 boundary */
> +       uint16_t _buf[MAX_PACK_LEN/2 + 1];
> +       uint8_t *buf = (uint8_t*)_buf;
>
> Unfortunately gcc is too stupid to not generate bigger code for this.
> So I went with slapping ALIGN4 on the byte array instead.
>
> The fix is here:
> http://busybox.net/downloads/fixes-1.16.1/busybox-1.16.1-dnsd.patch
>   


-- 
Lars Reemts, Lange Wieke 1, 26817 Rhauderfehn, Germany
Tel: +49 4952 942290, Fax: +49 4952 942291
mailto: lars at reemts.de