upstream/bind9
1
0
Fork 0
mirror of https://github.com/isc-projects/bind9.git synced 2026-06-08 21:42:06 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

Remove ns_client_t .shuttingdown member

Browse source 
The way the ns_client_t .shuttingdown member was practically dead code.
The .shuttingdown would be set to true only in ns__client_put() function
meaning that we have detached from all ns_client_t .*handles and the
ns_client_t object being freed:

    client->magic = 0;
    client->shuttingdown = true;
    [...]
    isc_mem_put(manager->ctx, client, sizeof(*client))

Meanwhile the ns_client_t object is accessed like this:

    isc_nmhandle_detach(&client->fetchhandle);

    client->query.attributes &= ~NS_QUERYATTR_RECURSING;
    client->state = NS_CLIENTSTATE_WORKING;

    qctx_init(client, &devent, 0, &qctx);

    client_shuttingdown = ns_client_shuttingdown(client);
    if (fetch_canceled || fetch_answered || client_shuttingdown) {
        [...]
    }

Even if the isc_nmhandle_detach(...) was the last handle detach, it
would mean that immediatelly, after calling the isc_nmhandle_detach(),
we would be causing use-after-free, because the ns_client_t is
immediatelly destroyed after setting .shuttingdown to true.

The similar code in the query_hookresume() already noticed this:

    /*
     * This event is running under a client task, so it's safe to detach
     * the fetch handle.  And it should be done before resuming query
     * processing below, since that may trigger another recursion or
     * asynchronous hook event.
     */
This commit is contained in:
Ondřej Surý 2022-03-23 16:16:53 +01:00
parent 9de10cd153
commit 1f35977423
3 changed files with 1 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
lib/ns/client.c
View file

@ -1672,7 +1672,6 @@ ns__client_put_cb(void *client0) {
client_extendederror_reset(client);
client->magic = 0;
client->shuttingdown = true;
isc_mem_put(manager->mctx, client->sendbuf, NS_CLIENT_SEND_BUFFER_SIZE);
if (client->opt != NULL) {
@ -2364,11 +2363,6 @@ cleanup:
return (result);
}
bool
ns_client_shuttingdown(ns_client_t *client) {
return (client->shuttingdown);
}
/***
*** Client Manager
***/

7
lib/ns/include/ns/client.h
View file

@ -172,7 +172,6 @@ struct ns_client {
ns_clientstate_t state;
int nupdates;
bool nodetach;
bool shuttingdown;
unsigned int attributes;
dns_view_t *view;
dns_dispatch_t *dispatch;
@ -316,12 +315,6 @@ ns_client_drop(ns_client_t *client, isc_result_t result);
* will be sent.
*/
bool
ns_client_shuttingdown(ns_client_t *client);
/*%<
* Return true iff the client is currently shutting down.
*/
isc_result_t
ns_client_replace(ns_client_t *client);
/*%<

4
lib/ns/query.c
View file

@ -6121,7 +6121,6 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
ns_client_t *client = NULL;
bool fetch_canceled = false;
bool fetch_answered = false;
bool client_shuttingdown = false;
isc_logcategory_t *logcategory = NS_LOGCATEGORY_QUERY_ERRORS;
isc_result_t result;
int errorloglevel;
@ -6218,8 +6217,7 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
*/
qctx_init(client, &devent, 0, &qctx);
client_shuttingdown = ns_client_shuttingdown(client);
if (fetch_canceled || fetch_answered || client_shuttingdown) {
if (fetch_canceled || fetch_answered) {
/*
* We've timed out or are shutting down. We can now
* free the event and other resources held by qctx, but