On Monday, 26. June 2006 15:11, Simon Kelley wrote:
> Daniel Hamlin wrote:
> > There appears to be a bug in 2.31 (and earlier), that mishandles requests
> > forwarded by a dhcp relay. In this packet capture, 192.168.1.196 is the
> > DHCP server, and 192.168.0.1 is the router (Nortel Passport 8600).
> > Notice the response is sent back to the wrong port on the relay.
> > According to "The DHCP Handbook", the response to a relay should go back
> > to port 67, not 68 as dnsmasq is currently doing.
>
> Your analysis is spot on: this code was changed around 2.28 to honour
> the source port in the request from the relay, rather then always send
> to port 68. The aim was to allow use of non-standard ports by relays,
> but it breaks if the relay sets port 67 as the source for the relayed
> packets, as in this case. It's probably best just to revert this change,
> and always send to port 68.
Although I have no idea what this really is about, I have an opinion. ;-)
Just to be sure: The change was made around 2.28 to deal with a certain issue
(and it really solved the problem, isn't it?) and now it turned out that it
breaks another thing - is that correct?
If that's correct, then my idea/suggestion is: Simon, please don't revert it
completely, make it a compile-time option instead. This would still have the
disadvantage that one cannot have a solution for both problems at the same
time, but this will hopefully happen very rarely.
> Cheers,
>
> Simon.
>
> > It appears that the bug is in dhcp.c line 229:
> >
> > if (mess->giaddr.s_addr)
> > {
> > /* Send to BOOTP relay */
> > if (!dest.sin_port)
> > dest.sin_port = htons(DHCP_SERVER_PORT);
> > dest.sin_addr = mess->giaddr;
> > }
E.g. like so: ;-)
if (mess->giaddr.s_addr)
{
/* Send to BOOTP relay */
#ifdef ALLOW_NONSTD_RELAY_PORTS
if (!dest.sin_port)
#endif
dest.sin_port = htons(DHCP_SERVER_PORT);
dest.sin_addr = mess->giaddr;
}
Then put an option into the configure defintions,
like "--allow-nonstd-relay-ports" and explain somewhere that this breaks the
standard behaviour.
Or name it e.g. "--allow-nonstd-relay-ports_breaks-std-behaviour". ;-)
Otherwise, reverting the change could cause a regression for those whose
relays use non-standard ports...
Cheers
Dirk
