[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