[Dnsmasq-discuss] Question on dnsmasq code, which may occur a bad-free

[Simon Kelley]

On 21/09/2024 12:08, 胡义臻 wrote:
> I'm renmingshuai's successor, a beginner with dnsmasq, and this e-mail 
> is follow-up to
> 
> https://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2024q3/017664.html <https://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2024q3/017664.html>
> 
> Question 1: Why does the dhcp_reply function add the stack variable 
> netid to daemon->dhcp_conf->netid->list, which is a global variable?
> 
> dhcp_reply
> 
> if (config)
> 
> {
> 
> struct dhcp_netid_list *list;
> 
> for (list = config->netid; list; list = list->next)
> 
> {
> 
> list->list->next = netid;
> 
> netid = list->list;
> 
> }
> 
> }
> 
> }
> 
> When we find config, 'list->list->next = netid;' add the stack variable 
> netid to the end of the linked list daemon->dhcp_conf->netid->list. 
> According to my understanding, the code is intended to add the first 
> node of config->netid->list to the head of netid. The purpose of 
> 'list->list->next = netid;' is to cache netid so that after 'netid = 
> list->list;' the new netid->next is the original netid. Do I understand 
> that right?
> 
> If I understand this correctly, the issue is that we add the stack 
> variable (for example, netid = &iface_id;, the &iface_id is a stack 
> address) to daemon->dhcp_conf->netid->list, and when dnsmasq receives 
> the SIGHUP signal, 'clear_dynamic_conf' will free the memory of 
> daemon->dhcp_conf->netid. In this case, daemon->dhcp_conf->netid in 
> stack space is freed as a pointer. If the pointer is not an address 
> returned by malloc, a bad-free error may occur, resulting in a 
> segmentation fault.
> 
> The coredump is as follows:
> 
> [New LWP 2925451]
> 
> [Thread debugging using libthread_db enabled]
> 
> Using host libthread_db library "/usr/lib64/libthread_db.so.1".
> 
> Core was generated by `dnsmasq --no-hosts --no-resolv 
> --pid-file=/var/lib/neutron/dhcp/d7435322-bf42-4'.
> 
> Program terminated with signal SIGSEGV, Segmentation fault.
> 
> #0 0x0000151e5168b0be in free () from /usr/lib64/libc.so.6
> 
> (gdb) bt
> 
> #0 0x0000151e5168b0be in free () from /usr/lib64/libc.so.6
> 
> #1 0x00005651025a08f0 in dhcp_netid_free (nid=0xc08b255501ae7f00) at 
> option.c:1022
> 
> #2 dhcp_netid_list_free (netid=0x0) at option.c:1053
> 
> #3 dhcp_config_free (config=0x565104124bc0) at option.c:1071
> 
> #4 0x00005651025aa011 in clear_dynamic_conf () at option.c:5204
> 
> #5 reread_dhcp () at option.c:5245
> 
> #6 0x00005651025b2c53 in clear_cache_and_reload (now=1722067132) at 
> dnsmasq.c:1699
> 
> #7 0x00005651025960ac in async_event (now=1722067132, pipe=15) at 
> dnsmasq.c:1449
> 
> #8 main (argc=<optimized out>, argv=<optimized out>) at dnsmasq.c:1192
> 
> (gdb)
> 
> Question 2: Why does the dhcp_reply function use two netid linked lists: 
> netid and tagif_netid? Can't we just use one?
> 
> Through code review, my understanding is as follows: netid stores 
> various tags. tagif_netid matches the content of the --tag-if option 
> with the tag saved in netid. If the matching is successful, the tag in 
> 'set:' is added and returned to tagif_netid. But why divide it into two 
> linked lists? Can't I directly return it to netid?
> 
> The reason I'm asking this question is, I want to put all the similar
> 
> list->list->next = netid;
> 
> netid = list->list;
> 
> change to
> 
> netid = dhcp_netid_create(list->list->net, netid);
> 
> The purpose of this is to avoid adding the stack variable netid to any 
> global variable. Then call
> 
> dhcp_netid_free(netid);
> 
> before the function return to free all the memory that has been 
> malloced.(I plan to change return to goto to avoid manually freeing 
> memory at each return)
> 
> If both the netid and tagif_netid exist, the free logic is complex.
> 
> If question one is indeed a problem, is the above modification 
> reasonable? Is there a better modification?
> 
> Question 3: Are there any common test cases for dnsmasq?
> 
> I found this test case on github, but it covered so little that I wanted 
> to find a comprehensive use case to ensure that my changes were correct 
> and did not introduce problems.
> 
> https://github.com/InfrastructureServices/dnsmasq-tests 
> <https://github.com/InfrastructureServices/dnsmasq-tests>
> 
> 
> 

This code is not an example to be followed. Understanding what's going 
on needs a bit of history.

Various dhcp configuration options can take zero or more "filter" tags 
and zero or more "set" tags, which are added to the set of active tags 
and used to filter dhcp configurations later in processing.

For instance you can filter a dhcp-host based in the network interface, 
and set a tag that controls which options are sent by dhcp-option

dhcp-host=tag:eth0,set:specialdns,00:11:22:33:44:55

dhcp-option=tag:specialdns,3,3.4.5.6

will send a special DNS server to MAC address 00:11:22:33:44:55 when 
it's connecting via eth0.

This used to be more limited: the number of "set" tags was limited to 
zero or one.

The "filter" tags are represented by a linked list of struct netid and 
the "set% tag was a single struct netid.

At run time, the dhcp code builds a linked list of all the active tags 
using the ->next filed of the "set".

At some point, the ability to have more than one "set" tag was added. 
Since the ->next filed of the netid structures was already in use as 
workspace, the single struct netid was replaced by a linked list of 
struct netid_list, each of which pointed to one struct netid.

None of this configuration could be altered at runtime, so none of this 
ever had to be freed.

At some point the ability to re-read configuration was added, so now old 
configs needed to be freed. The code to do this has a bug.
dhcp_netid_list_free() follows the linked list and frees each struct 
netid_list and the struct netid it points to, but it frees the struct 
netid by calling dhcp_netid_free(), which is meant to free the immutable 
"filter" tags, and follows the ->next pointers. in the "set" tags the 
->next pointers are dhcp processing workspace. Outside of dhcp_reply() 
they are not valid. Hence the crashes.

The fix is to change dhcp_netid_list_free() to

static void dhcp_netid_list_free(struct dhcp_netid_list *netid)
{
   while (netid)
     {
       struct dhcp_netid_list *tmplist = netid;
       netid = netid->next;
       /* Note: don't use dhcp_netid_free() here, since that
	 frees a list linked on netid->next. Where a netid_list
	 is used that's because the the ->next pointers in the
	 netids are being used to temporarily construct
	 a list of valid tags. */
       free(tmplist->list->net);
       free(tmplist->list);
       free(tmplist);
     }
}

I'll commit that change as soon as I've sent this.


Cheers,

Simon.