[Dnsmasq-discuss] [PATCH] Fix alignment issue

Simon Kelley simon at thekelleys.org.uk
Mon Aug 6 14:33:18 BST 2007


Alex Landau wrote:
> --- "richardvoigt at gmail.com" <richardvoigt at gmail.com> wrote:
> 
>> On 8/5/07, Alex Landau <landau_alex at yahoo.com> wrote:
>>> Hi All,
>>>
>>> I compiled dnsmasq for Blackfin (an embedded MMU-less CPU running uClinux)
>>> and
>>> encountered the following problem: whenever dnsmasq is about to send a DNS
>>> reply it gets
>>> a SIGBUS due to an unaligned memory access.
>>>
>>> I traced the issue and the patch below fixes that.
>>> The problem was in the cache_insert() function that receives (all_addr
>>> *addr) and
>>> memcpy's it somewhere else (at the "if (addr)" statement near the end).
>>> With -O2, gcc
>>> sees that addr is a pointer to a struct at least 4 bytes in size and
>>> replaces the memcpy
>>> with a single 4-byte memory fetch and memory store (4-byte since I
>>> disabled IPv6).
>>> Sometimes addr is not 4-bytes aligned (e.g. when called from
>>> extract_addresses(), the
>>> addr parameter is casted from p1 which is a char *). In my situation this
>>> caused addr to
>>> not be 4 bytes aligned which caused the SIGBUS.
>>>
>>> The patch below tells gcc that the struct all_addr is packed, so gcc will
>>> not assume it
>>> is aligned and will generate the call to memcpy instead of optimizing it.
>>
>> Does dnsmasq only compile on gcc?  AFAIK __attribute__ is not supported by
>> all compilers.
>>
> 
> Well, I don't know if other compilers are used. __attribute__ is indeed gcc specific (but
> IIRC, Intel compiler for Linux uses them too). For other compilers there should be
> pragmas or whatever that do the same. In my opinion, this is the cleanest solution to the
> alignment problem that might happen on any CPU not allowing unaligned memory access.
> 
> #if defined(__GNUC__)
> #define PACKED __attribute__((packed))
> #elsif defined(something_else)
> #define PACKED other stuff
> #else
> #define PACKED
> #endif
> 
> struct { .... } PACKED;

Thanks to Alex for finding and diagnosing this problem. My first
reaction was that it's a compiler problem: gcc shouldn't replace the
(alignment safe) memcpy with  a straight register copy. However, that's
wrong. It turns out that if you cast a pointer to <pointer to foo> which
isn't aligned for <foo> then the results are undefined.

http://lists.debian.org/debian-gcc/2005/08/msg00412.html

has details, including C-standard reference. So gcc is completely
entitled to assume that a struct all_addr * is properly aligned.

When I looked, there's an even more egregious problem where the same
pointer is just dereferenced in dns_doctor().

Given that the problem is my bad C, I think the best solution is to fix
that, rather than patch it up with compiler directives. I've put up a
test release at

http://www.thekelleys.org.uk/dnsmasq/test-releases/dnsmasq-2.40test20.tar.gz

which I think fixes things. Alex, I'd appreciate it if you could check
that on your Blackfin box. There are no changes from rc1, apart from the
alignment fix.


Cheers,

Simon.

> 
> BTW, After sending the mail, I ran a few more tests on the original version, and noticed
> that pings (that cause DNS lookups) from one computer (running Debian Etch) cause a
> SIGBUS, while pings from another (running Kubuntu Edgy) don't. Don't know what's the
> reason. Maybe the length of the name to be resolved...

Probably, there's a one-in-four chance that the alignment will be right
by chance.


> 
> Alex
> 
> 
>        
> ____________________________________________________________________________________
> Pinpoint customers who are looking for what you sell. 
> http://searchmarketing.yahoo.com/
> 
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss at lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
> 




More information about the Dnsmasq-discuss mailing list