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

[胡义臻]

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

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
















-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20240921/ceddfcf6/attachment.htm>