From ac110848296c18b3a3bfaa89bdee2dd00755b36f Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 26 May 2016 12:11:00 +1000 Subject: [PATCH] 4374. [bug] Use SAVE/RESTORE macros in query.c to reduce the probability of reference counting errors as seen in 4365. [RT #42405] --- CHANGES | 4 + bin/named/query.c | 232 +++++++++++++++++++--------------------------- 2 files changed, 99 insertions(+), 137 deletions(-) diff --git a/CHANGES b/CHANGES index fb77ce6736..8641170c45 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 599de05d92..b2a7650b63 100644 --- 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) {