[Dnsmasq-discuss] Git: Is first dhcp.c address_available() for/if code correct?

[Simon Kelley]

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 <simon at thekelleys.org.uk>
>> 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;
>> }
> 
>