On 9/24/25 12:07, Matthias Andree wrote:
Simon,
looking at dhcp.c as of this commit, and I see it's unchanged from v2.91:
commit ee09f0655c0a4347a72d2bf9b7231ff158a13f53 (HEAD-> master,
origin/master, origin/HEAD)
Author: Simon Kelley <[email protected]>
Date: Mon Sep 1 22:35:02 2025 +0100
Optimise tftp.
I wonder if the if() clause (next to last line below) is correct, or is
in the right place.
That's currently line #699.
The concern is that everything in the
if(taddr.s_addr==context->router.s_addr)
expression is independent from tmp in the enlosing for(;;) loop, and I
cannot fathom how this would make sense.
I suppose a reasonable compiler can decide the first for(tmp=context;
tmp; tmp=tmp->current) does not cause observable effects (unless
undefined behavior) and just discard the for loop.
Either we want to look at just the context->router.s_addr, then we don't
need the for(),
or the if() clause needs to look at something with "tmp"
to make sense for both lines.
Definitely a bug. The code is trying to disallow offering an IP address
which is in use by the DHCP server, and it works fine for the simple
case. It may fail if there are multiple subnets on the same broadcast
domain.
e.g.
dhcp-range=192.168.1.1,192.168.1.100
dhcp-range=192.168.2.1,192.168.2.100
eth0 on the dnsmasq server has addresses 192.168.1.1 and 192.168.2.1
and a DHCPDISCOVER arrives on eth0
we should not offer 192.168.1.1 or 192.168.2.1, but the code as it
stands will only disable one of those, which one is indeterminate.
This is not a major bug, or a security problem.
I've just committed the fix.
Also, I am confused reading the undocumented fields of struct
dhcp_context {} without reading more than the declaration - it has one
"current" and one "next" pointer and we iterate through "current", which
isn't very typical from just reading it, but done in many places inside
the code so I presume the pointer fields are only named confusingly, so
I cannot assess what impact this inconsistency has.
The ->current fields are temporary working space that links all the
dhcp-range configs which are valid for the current DHCP transaction in a
linked list. Said linked list is passed into the address_available()
function in the "context" argument.
Cheers,
Simon.>
structdhcp_context*address_available(structdhcp_context*context,
structin_addrtaddr,
structdhcp_netid*netids)
{
/* Check is an address is OK for this network, check all
possible ranges. Make sure that the address isn't in use
by the server itself. */
unsignedintstart, end, addr=ntohl(taddr.s_addr);
structdhcp_context*tmp;
for(tmp=context; tmp; tmp=tmp->current)
if(taddr.s_addr==context->router.s_addr) // <- QUESTIONABLE for/if
VARIABLE CONCORD
returnNULL;
for(tmp=context; tmp; tmp=tmp->current)
{
start=ntohl(tmp->start.s_addr);
end=ntohl(tmp->end.s_addr);
if(!(tmp->flags&(CONTEXT_STATIC|CONTEXT_PROXY)) &&
addr>=start&&
addr<=end&&
match_netid(tmp->filter, netids, 1))
returntmp;
}
returnNULL;
}
_______________________________________________
Dnsmasq-discuss mailing list
[email protected]
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
