Busybox 1.16.x dnsd alignment problems

[Lars Reemts]

> So I went with slapping ALIGN4 on the byte array instead.

The ALIGN4 doesn't compile for me. gcc complains not to know the ALIGN4
macro. Had to change it to:
uint8_t buf[MAX_PACK_LEN + 1] __attribute__ ((aligned (4)));

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