diff --git a/CHANGES b/CHANGES index 3b7473335c..6ef135d20e 100644 --- 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 d780671eb5..ad1266e614 100644 --- a/bin/named/query.c +++ b/bin/named/query.c @@ -165,6 +165,19 @@ client_trace(ns_client_t *client, int level, const char *message) { #define PENDINGOK(x) (((x) & DNS_DBFIND_PENDINGOK) != 0) +/* + * 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; @@ -1436,8 +1449,7 @@ query_addadditional(void *arg, dns_name_t *name, dns_rdatatype_t qtype) { } else need_addname = ISC_TRUE; ISC_LIST_APPEND(fname->list, rdataset, link); - trdataset = rdataset; - rdataset = NULL; + SAVE(trdataset, rdataset); added_something = ISC_TRUE; /* * Note: we only add SIGs if we've added the type they cover, @@ -3164,16 +3176,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; @@ -3204,8 +3212,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 @@ -3216,10 +3222,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); } /* @@ -4055,8 +4061,7 @@ rpz_rrset_find(ns_client_t *client, dns_rpz_type_t rpz_type, st->r.db = NULL; if (*rdatasetp != NULL) query_putrdataset(client, rdatasetp); - *rdatasetp = st->r.r_rdataset; - st->r.r_rdataset = NULL; + RESTORE(*rdatasetp, st->r.r_rdataset); result = st->r.r_result; if (result == DNS_R_DELEGATION) { CTRACE(ISC_LOG_ERROR, "RPZ recursing"); @@ -4470,6 +4475,7 @@ rpz_rewrite_name(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, zone = NULL; db = NULL; node = NULL; + version = NULL; for (rpz = ISC_LIST_HEAD(client->view->rpz_zones); rpz != NULL; @@ -4605,24 +4611,21 @@ rpz_rewrite_name(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, dns_name_copy(rpz_qname, st->qname, NULL); if (*rdatasetp != NULL && dns_rdataset_isassociated(*rdatasetp)) { - dns_rdataset_t *trdataset; + dns_rdataset_t *trdataset = NULL; - 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.node = node; - node = NULL; - st->m.db = db; - db = NULL; - st->m.version = version; - st->m.zone = zone; - zone = NULL; + SAVE(st->m.node, node); + SAVE(st->m.db, db); + SAVE(st->m.version, version); + SAVE(st->m.zone, zone); } } @@ -5499,12 +5502,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); @@ -5778,32 +5781,27 @@ 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; if (event->node != NULL) dns_db_detachnode(event->db, &event->node); + SAVE(rpz_st->r.db, event->db); rpz_st->r.r_type = event->qtype; - rpz_st->r.r_rdataset = event->rdataset; + SAVE(rpz_st->r.r_rdataset, event->rdataset); query_putrdataset(client, &event->sigrdataset); } else { authoritative = ISC_FALSE; qtype = event->qtype; - db = event->db; - node = event->node; - rdataset = event->rdataset; - sigrdataset = event->sigrdataset; + RESTORE(db, event->db); + RESTORE(node, event->node); + RESTORE(rdataset, event->rdataset); + RESTORE(sigrdataset, event->sigrdataset); } INSIST(rdataset != NULL); @@ -5934,9 +5932,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 { @@ -6173,16 +6172,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; @@ -6204,20 +6198,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_NXDOMAIN: @@ -6408,9 +6399,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; } @@ -6473,17 +6465,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; @@ -6508,8 +6495,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 @@ -6521,12 +6506,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 @@ -6621,10 +6606,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) { @@ -6661,15 +6644,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) { @@ -6938,10 +6917,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) { @@ -6975,10 +6952,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 @@ -6988,11 +6962,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) { @@ -7508,13 +7481,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) {