[peter at p12n.org: dhcp 3.0.3 theoretical alignment bug]

[David W. Hankins]

On Tue, Aug 30, 2005 at 09:10:13PM +1000, Andrew Pollock wrote:
> Spent some time investigating a bug in 3.0.2 that seemed to be partially
> fixed (but without anything appropriate that I could see in the release
> notes in 3.0.3).

                        Changes since 3.0.2

- decode_udp_ip_header was changed so that the IP address was copied out
  to a variable, rather than referenced by a pointer.  This enforces 4-byte
  alignment of the 32-bit IP address value.  Thanks to a patch from Dr.
  Peter Poeml.

> So, I was helping someone track down a pointer alignment bug in dhcp
> 3.0.2, and it turned out to be fixed in dhcp 3.0.3.  However, there is
> another bug of exactly the same type, in the same function, *not* fixed
> in 3.0.3.  Why it does not trigger in the same situation, I don't know.
> 
> The 3.0.2 bug was in decode_udp_ip_header(): 'struct ip *ip', which was
> changed to a stack variable 'struct ip ip' and copied from the input
> buffer, for proper struct alignment.  The related variable 'struct
> udphdr *udp' was not changed in the same way and I think it should have
> been.

Mmm.  The 'buf' field starts out 2 bytes misaligned, the ip header is
32-bit aligned itself, so the the udp offset realigns it (and the bootp
payload is once again 32-bit aligned).

So, 32bit->2-16-bit-registers compiler tricks in the udp header area
would likely not work, no.

The reason no one else has seen this is most likely compiler flags.

> Practical result: the UDP checksum in the input buffer is no longer
> zeroed.  I didn't check to see if this mattered.

It may in the relay, I need to relearn how that works.

-- 
David W. Hankins		"If you don't do it right the first time,
Software Engineer			you'll just have to do it again."
Internet Systems Consortium, Inc.		-- Jack T. Hankins