[Dnsmasq-discuss] dnsmasq 2.25 crashing in some DHCP NAK cases
(and possible fix)
Simon Kelley
simon at thekelleys.org.uk
Sun Jan 22 13:10:22 GMT 2006
Lutz Pressler wrote:
> Hello,
>
> at least 2.25 (and probably since 2.23 as then sending a server identifier
> with DHCPNAK had been introduced)
I checked: it's only 2.25 which has this problem: 2.23 and 2.24 are OK.
The trigger for the problem was moving the call to narrow_context()
earlier in the code, to cope with interfaces which have more than one
address, each on a different subnet. That happened in 2.25.
> crashes "in case of a machine moving whilst
> it has a lease". Reason (rfc2131.c line 631ff):
>
> if (!(context = narrow_context(context, mess->yiaddr)))
> {
> /* If a machine moves networks whilst it has a lease, we catch that here. */
> message = _("wrong network");
> /* ensure we broadcast NAK */
> unicast_dest = 0;
> }
>
> ... so in this case "context" is NULL.
> But in the section starting line 664 "context" is dereferenced:
>
> if (message)
> {
> log_packet("NAK", &mess->yiaddr, chaddr, iface_name, message);
>
> mess->siaddr.s_addr = mess->yiaddr.s_addr = 0;
> bootp_option_put(mess, NULL, NULL);
> p = option_put(p, end, OPTION_MESSAGE_TYPE, 1, DHCPNAK);
> --> p = option_put(p, end, OPTION_SERVER_IDENTIFIER, INADDRSZ, ntohl(context->local.s_addr));
> p = option_put_string(p, end, OPTION_MESSAGE, message);
> /* This fixes a problem with the DHCP spec, broadcasting a NAK to a host on
> a distant subnet which unicast a REQ to us won't work. */
> if (!unicast_dest || mess->giaddr.s_addr != 0 ||
> --> mess->ciaddr.s_addr == 0 || is_same_net(context->local, mess->ciaddr, context->netmask))
> {
> mess->flags |= htons(0x8000); /* broadcast */
> mess->ciaddr.s_addr = 0;
> }
> }
>
> The second case is no problem because of lazy evaluation as
> "unicast_dest" is zero and tested earlier - but the first case is:
> The attached patch changes "ntohl(context->local.s_addr)" into
> "context ? ntohl(context->local.s_addr) : NULL" and fixes the
> problem for me - but probably sending a NULL server identifier
> is not correct. As I understand RFC 2131, the option has to be
> included, but maybe one should use a local address of the
> interface the request came in?
>
That's a great analysis, thanks. The correct way to cope with
context == NULL as it use the value of context->local from before
the call to narrow_context(). That will give a good value for an address
of the local interface.
Since this is a relatively bad bug, and only in 2.25 which is a week
old, I think the best course is to release 2.26, and deprecate 2.25.
> Have a nice day,
Thanks for your input.
Cheers,
Simon.
More information about the Dnsmasq-discuss
mailing list