]> git.ipfire.org Git - thirdparty/bind9.git/commit
Remove ns_client_t .shuttingdown member
authorOndřej Surý <ondrej@isc.org>
Wed, 23 Mar 2022 15:16:53 +0000 (16:16 +0100)
committerOndřej Surý <ondrej@isc.org>
Fri, 25 Mar 2022 09:38:35 +0000 (10:38 +0100)
commit1f3597742335cf43a0bc08a75691a96faae02e4f
treec8f1160ed34005bbaae337449539576fbc340b6d
parent9de10cd15307d85996dd144c3464a6359b90a20e
Remove ns_client_t .shuttingdown member

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.
     */
lib/ns/client.c
lib/ns/include/ns/client.h
lib/ns/query.c