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.
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
