4374. [bug] Use SAVE/RESTORE macros in query.c to reduce the

                        probability of reference counting errors as seen
                        in 4365. [RT #42405]

diff --git a/CHANGES b/CHANGES
index fb77ce6736d7682189017d102faab0006f447fa8..8641170c453918f265aa0a9df13e6c8f9f532258 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,7 @@
+4374.  [bug]           Use SAVE/RESTORE macros in query.c to reduce the
+                       probability of reference counting errors as seen
+                       in 4365. [RT #42405]
+
 4373.  [bug]           Address undefined behaviour in getaddrinfo. [RT #42479]
 
 4372.  [bug]           Address undefined behaviour in libt_api. [RT #42480]

diff --git a/bin/named/query.c b/bin/named/query.c
index 599de05d9218d053e654a392af1451ea969a49bc..b2a7650b63f57ee72e8af80ebdd457ddc4ea9919 100644 (file)
--- a/bin/named/query.c
+++ b/bin/named/query.c
@@ -178,6 +178,19 @@ client_trace(ns_client_t *client, int level, const char *message) {
 
 #define SFCACHE_CDFLAG 0x1
 
+/*
+ * These have the same semantics as:
+ *
+ *     foo_attach(b, a);
+ *     foo_detach(&a);
+ *
+ * without the locking and magic testing.
+ *
+ * We use SAVE and RESTORE as that shows the operation being performed.
+ */
+#define SAVE(a, b) do { INSIST(a == NULL); a = b; b = NULL; } while (0)
+#define RESTORE(a, b) SAVE(a, b)
+
 typedef struct client_additionalctx {
        ns_client_t *client;
        dns_rdataset_t *rdataset;
@@ -3242,16 +3255,12 @@ query_addbestns(ns_client_t *client) {
                        goto cleanup;
                if (USECACHE(client)) {
                        query_keepname(client, fname, dbuf);
-                       zdb = db;
-                       zfname = fname;
-                       fname = NULL;
-                       zrdataset = rdataset;
-                       rdataset = NULL;
-                       zsigrdataset = sigrdataset;
-                       sigrdataset = NULL;
                        dns_db_detachnode(db, &node);
+                       SAVE(zdb, db);
+                       SAVE(zfname, fname);
+                       SAVE(zrdataset, rdataset);
+                       SAVE(zsigrdataset, sigrdataset);
                        version = NULL;
-                       db = NULL;
                        dns_db_attach(client->view->cachedb, &db);
                        is_zone = ISC_FALSE;
                        goto db_find;
@@ -3282,8 +3291,6 @@ query_addbestns(ns_client_t *client) {
 
        if (use_zone) {
                query_releasename(client, &fname);
-               fname = zfname;
-               zfname = NULL;
                /*
                 * We've already done query_keepname() on
                 * zfname, so we must set dbuf to NULL to
@@ -3294,10 +3301,10 @@ query_addbestns(ns_client_t *client) {
                query_putrdataset(client, &rdataset);
                if (sigrdataset != NULL)
                        query_putrdataset(client, &sigrdataset);
-               rdataset = zrdataset;
-               zrdataset = NULL;
-               sigrdataset = zsigrdataset;
-               zsigrdataset = NULL;
+               
+               RESTORE(fname, zfname);
+               RESTORE(rdataset, zrdataset);
+               RESTORE(sigrdataset, zsigrdataset);
        }
 
        /*
@@ -4646,7 +4653,7 @@ rpz_save_p(dns_rpz_st_t *st, dns_rpz_zone_t *rpz, dns_rpz_type_t rpz_type,
           dns_dbnode_t **nodep, dns_rdataset_t **rdatasetp,
           dns_dbversion_t *version)
 {
-       dns_rdataset_t *trdataset;
+       dns_rdataset_t *trdataset = NULL;
 
        rpz_match_clear(st);
        st->m.rpz = rpz;
@@ -4655,25 +4662,22 @@ rpz_save_p(dns_rpz_st_t *st, dns_rpz_zone_t *rpz, dns_rpz_type_t rpz_type,
        dns_name_copy(p_name, st->p_name, NULL);
        st->m.prefix = prefix;
        st->m.result = result;
-       st->m.zone = *zonep;
-       *zonep = NULL;
-       st->m.db = *dbp;
-       *dbp = NULL;
-       st->m.node = *nodep;
-       *nodep = NULL;
+       SAVE(st->m.zone, *zonep);
+       SAVE(st->m.db, *dbp);
+       SAVE(st->m.node, *nodep);
        if (*rdatasetp != NULL && dns_rdataset_isassociated(*rdatasetp)) {
                /*
                 * Save the replacement rdataset from the policy
                 * and make the previous replacement rdataset scratch.
                 */
-               trdataset = st->m.rdataset;
-               st->m.rdataset = *rdatasetp;
-               *rdatasetp = trdataset;
+               SAVE(trdataset, st->m.rdataset);
+               SAVE(st->m.rdataset, *rdatasetp);
+               SAVE(*rdatasetp, trdataset);
                st->m.ttl = ISC_MIN(st->m.rdataset->ttl, rpz->max_policy_ttl);
        } else {
                st->m.ttl = ISC_MIN(DNS_RPZ_TTL_DEFAULT, rpz->max_policy_ttl);
        }
-       st->m.version = version;
+       SAVE(st->m.version, version);
 }
 
 /*
@@ -6165,12 +6169,12 @@ dns64_aaaaok(ns_client_t *client, dns_rdataset_t *rdataset,
                             aaaaok, count)) {
                for (i = 0; i < count; i++) {
                        if (aaaaok != NULL && !aaaaok[i]) {
-                               client->query.dns64_aaaaok = aaaaok;
+                               SAVE(client->query.dns64_aaaaok, aaaaok);
                                client->query.dns64_aaaaoklen = count;
                                break;
                        }
                }
-               if (i == count && aaaaok != NULL)
+               if (aaaaok != NULL)
                        isc_mem_put(client->mctx, aaaaok,
                                    sizeof(isc_boolean_t) * count);
                return (ISC_TRUE);
@@ -6612,16 +6616,11 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
 
                        is_zone = rpz_st->q.is_zone;
                        authoritative = rpz_st->q.authoritative;
-                       zone = rpz_st->q.zone;
-                       rpz_st->q.zone = NULL;
-                       node = rpz_st->q.node;
-                       rpz_st->q.node = NULL;
-                       db = rpz_st->q.db;
-                       rpz_st->q.db = NULL;
-                       rdataset = rpz_st->q.rdataset;
-                       rpz_st->q.rdataset = NULL;
-                       sigrdataset = rpz_st->q.sigrdataset;
-                       rpz_st->q.sigrdataset = NULL;
+                       RESTORE(zone, rpz_st->q.zone);
+                       RESTORE(node, rpz_st->q.node);
+                       RESTORE(db, rpz_st->q.db);
+                       RESTORE(rdataset, rpz_st->q.rdataset);
+                       RESTORE(sigrdataset, rpz_st->q.sigrdataset);
                        qtype = rpz_st->q.qtype;
 
                        rpz_st->r.db = event->db;
@@ -6649,16 +6648,12 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
 #endif
                        qtype = client->query.redirect.qtype;
                        INSIST(client->query.redirect.rdataset != NULL);
-                       rdataset = client->query.redirect.rdataset;
-                       client->query.redirect.rdataset = NULL;
-                       sigrdataset = client->query.redirect.sigrdataset;
-                       client->query.redirect.sigrdataset = NULL;
-                       db = client->query.redirect.db;
-                       client->query.redirect.db = NULL;
-                       node = client->query.redirect.node;
-                       client->query.redirect.node = NULL;
-                       zone = client->query.redirect.zone;
-                       client->query.redirect.zone = NULL;
+                       RESTORE(rdataset, client->query.redirect.rdataset);
+                       RESTORE(sigrdataset,
+                               client->query.redirect.sigrdataset);
+                       RESTORE(db, client->query.redirect.db);
+                       RESTORE(node, client->query.redirect.node);
+                       RESTORE(zone, client->query.redirect.zone);
                        authoritative = client->query.redirect.authoritative;
 
                        /*
@@ -6880,9 +6875,10 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                dns_db_detach(&db);
                        if (zone != NULL)
                                dns_zone_detach(&zone);
-                       version = tversion;
-                       db = tdb;
-                       zone = tzone;
+                       version = NULL;
+                       RESTORE(version, tversion);
+                       RESTORE(db, tdb);
+                       RESTORE(zone, tzone);
                        is_zone = ISC_TRUE;
                        result = ISC_R_SUCCESS;
                } else {
@@ -7140,16 +7136,11 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                        rpz_st->q.qtype = qtype;
                        rpz_st->q.is_zone = is_zone;
                        rpz_st->q.authoritative = authoritative;
-                       rpz_st->q.zone = zone;
-                       zone = NULL;
-                       rpz_st->q.db = db;
-                       db = NULL;
-                       rpz_st->q.node = node;
-                       node = NULL;
-                       rpz_st->q.rdataset = rdataset;
-                       rdataset = NULL;
-                       rpz_st->q.sigrdataset = sigrdataset;
-                       sigrdataset = NULL;
+                       SAVE(rpz_st->q.zone, zone);
+                       SAVE(rpz_st->q.db, db);
+                       SAVE(rpz_st->q.node, node);
+                       SAVE(rpz_st->q.rdataset, rdataset);
+                       SAVE(rpz_st->q.sigrdataset, sigrdataset);
                        dns_name_copy(fname, rpz_st->fname, NULL);
                        rpz_st->q.result = result;
                        client->query.attributes |= NS_QUERYATTR_RECURSING;
@@ -7179,20 +7170,17 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                        rpz_clean(&zone, &db, &node, NULL);
                        if (rpz_st->m.rdataset != NULL) {
                                query_putrdataset(client, &rdataset);
-                               rdataset = rpz_st->m.rdataset;
-                               rpz_st->m.rdataset = NULL;
+                               RESTORE(rdataset, rpz_st->m.rdataset);
                        } else if (rdataset != NULL &&
                                   dns_rdataset_isassociated(rdataset)) {
                                dns_rdataset_disassociate(rdataset);
                        }
-                       node = rpz_st->m.node;
-                       rpz_st->m.node = NULL;
-                       db = rpz_st->m.db;
-                       rpz_st->m.db = NULL;
-                       version = rpz_st->m.version;
-                       rpz_st->m.version = NULL;
-                       zone = rpz_st->m.zone;
-                       rpz_st->m.zone = NULL;
+                       version = NULL;
+
+                       RESTORE(node, rpz_st->m.node);
+                       RESTORE(db, rpz_st->m.db);
+                       RESTORE(version, rpz_st->m.version);
+                       RESTORE(zone, rpz_st->m.zone);
 
                        switch (rpz_st->m.policy) {
                        case DNS_RPZ_POLICY_TCP_ONLY:
@@ -7406,9 +7394,10 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                                dns_db_detach(&db);
                                        if (zone != NULL)
                                                dns_zone_detach(&zone);
-                                       version = tversion;
-                                       db = tdb;
-                                       zone = tzone;
+                                       version = NULL;
+                                       RESTORE(version, tversion);
+                                       RESTORE(db, tdb);
+                                       RESTORE(zone, tzone);
                                        authoritative = ISC_TRUE;
                                        goto db_find;
                                }
@@ -7471,17 +7460,12 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                 * use it instead.
                                 */
                                query_keepname(client, fname, dbuf);
-                               zdb = db;
-                               zfname = fname;
-                               fname = NULL;
-                               zrdataset = rdataset;
-                               rdataset = NULL;
-                               zsigrdataset = sigrdataset;
-                               sigrdataset = NULL;
                                dns_db_detachnode(db, &node);
-                               zversion = version;
-                               version = NULL;
-                               db = NULL;
+                               SAVE(zdb, db);
+                               SAVE(zfname, fname);
+                               SAVE(zrdataset, rdataset);
+                               SAVE(zsigrdataset, sigrdataset);
+                               SAVE(zversion, version);
                                dns_db_attach(client->view->cachedb, &db);
                                is_zone = ISC_FALSE;
                                goto db_find;
@@ -7506,8 +7490,6 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                 *    static-stub zone.
                                 */
                                query_releasename(client, &fname);
-                               fname = zfname;
-                               zfname = NULL;
                                /*
                                 * We've already done query_keepname() on
                                 * zfname, so we must set dbuf to NULL to
@@ -7519,12 +7501,12 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                if (sigrdataset != NULL)
                                        query_putrdataset(client,
                                                          &sigrdataset);
-                               rdataset = zrdataset;
-                               zrdataset = NULL;
-                               sigrdataset = zsigrdataset;
-                               zsigrdataset = NULL;
-                               version = zversion;
-                               zversion = NULL;
+                               version = NULL;
+
+                               RESTORE(fname, zfname);
+                               RESTORE(rdataset, zrdataset);
+                               RESTORE(sigrdataset, zsigrdataset);
+                               RESTORE(version, zversion);
                                /*
                                 * We don't clean up zdb here because we
                                 * may still need it.  It will get cleaned
@@ -7620,10 +7602,8 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                query_putrdataset(client, &rdataset);
                        if (sigrdataset != NULL)
                                query_putrdataset(client, &sigrdataset);
-                       rdataset = client->query.dns64_aaaa;
-                       sigrdataset = client->query.dns64_sigaaaa;
-                       client->query.dns64_aaaa = NULL;
-                       client->query.dns64_sigaaaa = NULL;
+                       RESTORE(rdataset, client->query.dns64_aaaa);
+                       RESTORE(sigrdataset, client->query.dns64_sigaaaa);
                        if (fname == NULL) {
                                dbuf = query_getnamebuf(client);
                                if (dbuf == NULL) {
@@ -7660,15 +7640,11 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                         * Look to see if there are A records for this
                         * name.
                         */
-                       INSIST(client->query.dns64_aaaa == NULL);
-                       INSIST(client->query.dns64_sigaaaa == NULL);
-                       client->query.dns64_aaaa = rdataset;
-                       client->query.dns64_sigaaaa = sigrdataset;
+                       SAVE(client->query.dns64_aaaa, rdataset);
+                       SAVE(client->query.dns64_sigaaaa, sigrdataset);
                        client->query.dns64_ttl = dns64_ttl(db, version);
                        query_releasename(client, &fname);
                        dns_db_detachnode(db, &node);
-                       rdataset = NULL;
-                       sigrdataset = NULL;
                        type = qtype = dns_rdatatype_a;
                        rpz_st = client->query.rpz_st;
                        if (rpz_st != NULL) {
@@ -7837,23 +7813,18 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                          dns_nsstatscounter_nxdomainredirect_rlookup);
                                client->query.redirect.qtype = qtype;
                                INSIST(rdataset != NULL);
-                               client->query.redirect.rdataset = rdataset;
-                               client->query.redirect.sigrdataset =
-                                                                sigrdataset;
-                               client->query.redirect.db = db;
-                               client->query.redirect.node = node;
-                               client->query.redirect.zone = zone;
+                               SAVE(client->query.redirect.rdataset, rdataset);
+                               SAVE(client->query.redirect.sigrdataset,
+                                    sigrdataset);
+                               SAVE(client->query.redirect.db, db);
+                               SAVE(client->query.redirect.node, node);
+                               SAVE(client->query.redirect.zone, zone);
                                client->query.redirect.result = DNS_R_NXDOMAIN;
                                dns_name_copy(fname,
                                              client->query.redirect.fname,
                                              NULL);
                                client->query.redirect.authoritative =
                                        authoritative;
-                               db = NULL;
-                               node = NULL;
-                               zone = NULL;
-                               rdataset = NULL;
-                               sigrdataset = NULL;
                                goto cleanup;
                        }
                        if (tresult == ISC_R_SUCCESS) {
@@ -7955,22 +7926,17 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                if (tresult == DNS_R_CONTINUE) {
                        inc_stats(client,
                                  dns_nsstatscounter_nxdomainredirect_rlookup);
-                       client->query.redirect.db = db;
-                       client->query.redirect.node = node;
-                       client->query.redirect.zone = zone;
+                       SAVE(client->query.redirect.db, db);
+                       SAVE(client->query.redirect.node, node);
+                       SAVE(client->query.redirect.zone, zone);
                        client->query.redirect.qtype = qtype;
                        INSIST(rdataset != NULL);
-                       client->query.redirect.rdataset = rdataset;
-                       client->query.redirect.sigrdataset = sigrdataset;
+                       SAVE(client->query.redirect.rdataset, rdataset);
+                       SAVE(client->query.redirect.sigrdataset, sigrdataset);
                        client->query.redirect.result = DNS_R_NCACHENXDOMAIN;
                        dns_name_copy(fname, client->query.redirect.fname,
                                      NULL);
                        client->query.redirect.authoritative = authoritative;
-                       db = NULL;
-                       node = NULL;
-                       zone = NULL;
-                       rdataset = NULL;
-                       sigrdataset = NULL;
                        goto cleanup;
                }
                if (tresult == ISC_R_SUCCESS) {
@@ -8021,10 +7987,8 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                query_putrdataset(client, &rdataset);
                        if (sigrdataset != NULL)
                                query_putrdataset(client, &sigrdataset);
-                       rdataset = client->query.dns64_aaaa;
-                       sigrdataset = client->query.dns64_sigaaaa;
-                       client->query.dns64_aaaa = NULL;
-                       client->query.dns64_sigaaaa = NULL;
+                       RESTORE(rdataset, client->query.dns64_aaaa);
+                       RESTORE(sigrdataset, client->query.dns64_sigaaaa);
                        if (fname == NULL) {
                                dbuf = query_getnamebuf(client);
                                if (dbuf == NULL) {
@@ -8058,10 +8022,7 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                         * Look to see if there are A records for this
                         * name.
                         */
-                       INSIST(client->query.dns64_aaaa == NULL);
-                       INSIST(client->query.dns64_sigaaaa == NULL);
-                       client->query.dns64_aaaa = rdataset;
-                       client->query.dns64_sigaaaa = sigrdataset;
+
                        /*
                         * If the ttl is zero we need to workout if we have just
                         * decremented to zero or if there was no negative cache
@@ -8071,11 +8032,10 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                                client->query.dns64_ttl = rdataset->ttl;
                        else if (dns_rdataset_first(rdataset) == ISC_R_SUCCESS)
                                client->query.dns64_ttl = 0;
+                       SAVE(client->query.dns64_aaaa, rdataset);
+                       SAVE(client->query.dns64_sigaaaa, sigrdataset);
                        query_releasename(client, &fname);
                        dns_db_detachnode(db, &node);
-                       rdataset = NULL;
-                       sigrdataset = NULL;
-                       fname = NULL;
                        type = qtype = dns_rdatatype_a;
                        rpz_st = client->query.rpz_st;
                        if (rpz_st != NULL) {
@@ -8643,13 +8603,11 @@ query_find(ns_client_t *client, dns_fetchevent_t *event, dns_rdatatype_t qtype)
                         * Look to see if there are A records for this
                         * name.
                         */
-                       client->query.dns64_aaaa = rdataset;
-                       client->query.dns64_sigaaaa = sigrdataset;
                        client->query.dns64_ttl = rdataset->ttl;
+                       SAVE(client->query.dns64_aaaa, rdataset);
+                       SAVE(client->query.dns64_sigaaaa, sigrdataset);
                        query_releasename(client, &fname);
                        dns_db_detachnode(db, &node);
-                       rdataset = NULL;
-                       sigrdataset = NULL;
                        type = qtype = dns_rdatatype_a;
                        rpz_st = client->query.rpz_st;
                        if (rpz_st != NULL) {