Skip to content

Commit

Permalink
fix: dev: Lock and attach when returning zone stats
Browse files Browse the repository at this point in the history 
When returning zone statistics counters, the statistics sets are now attached while the zone is locked.  This addresses Coverity warnings CID 468720, 468728 and 468729.

Closes #4934

Merge branch '4934-lock-and-attach-when-return-zone-stats' into 'main'

See merge request isc-projects/bind9!9488
  • Loading branch information
@marka63
marka63 committed Dec 6, 2024
2 parents 6673835 + fb50a71 commit 3c720c6
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 48 deletions.
50 changes: 37 additions & 13 deletions bin/named/statschannel.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1301,6 +1301,9 @@ zone_xmlrender(dns_zone_t *zone, void *arg) {
stats_dumparg_t dumparg;
const char *ztype;
isc_time_t timestamp;
isc_stats_t *zonestats = NULL;
dns_stats_t *rcvquerystats = NULL;
dns_stats_t *dnssecsignstats = NULL;

statlevel = dns_zone_getstatlevel(zone);
if (statlevel == dns_zonestat_none) {
Expand Down Expand Up @@ -1365,14 +1368,11 @@ zone_xmlrender(dns_zone_t *zone, void *arg) {
}

if (statlevel == dns_zonestat_full) {
isc_stats_t *zonestats;
isc_stats_t *gluecachestats;
dns_stats_t *rcvquerystats;
dns_stats_t *dnssecsignstats;
uint64_t nsstat_values[ns_statscounter_max];
uint64_t gluecachestats_values[dns_gluecachestatscounter_max];

zonestats = dns_zone_getrequeststats(zone);
dns_zone_getrequeststats(zone, &zonestats);
if (zonestats != NULL) {
TRY0(xmlTextWriterStartElement(writer,
ISC_XMLCHAR "counters"));
Expand All @@ -1386,6 +1386,7 @@ zone_xmlrender(dns_zone_t *zone, void *arg) {
nsstat_values, ISC_STATSDUMP_VERBOSE));
/* counters type="rcode"*/
TRY0(xmlTextWriterEndElement(writer));
isc_stats_detach(&zonestats);
}

gluecachestats = dns_zone_getgluecachestats(zone);
Expand All @@ -1406,7 +1407,7 @@ zone_xmlrender(dns_zone_t *zone, void *arg) {
TRY0(xmlTextWriterEndElement(writer));
}

rcvquerystats = dns_zone_getrcvquerystats(zone);
dns_zone_getrcvquerystats(zone, &rcvquerystats);
if (rcvquerystats != NULL) {
TRY0(xmlTextWriterStartElement(writer,
ISC_XMLCHAR "counters"));
Expand All @@ -1421,9 +1422,10 @@ zone_xmlrender(dns_zone_t *zone, void *arg) {

/* counters type="qtype"*/
TRY0(xmlTextWriterEndElement(writer));
dns_stats_detach(&rcvquerystats);
}

dnssecsignstats = dns_zone_getdnssecsignstats(zone);
dns_zone_getdnssecsignstats(zone, &dnssecsignstats);
if (dnssecsignstats != NULL) {
/* counters type="dnssec-sign"*/
TRY0(xmlTextWriterStartElement(writer,
Expand Down Expand Up @@ -1456,13 +1458,23 @@ zone_xmlrender(dns_zone_t *zone, void *arg) {

/* counters type="dnssec-refresh"*/
TRY0(xmlTextWriterEndElement(writer));
dns_stats_detach(&dnssecsignstats);
}
}

TRY0(xmlTextWriterEndElement(writer)); /* zone */

return ISC_R_SUCCESS;
cleanup:
if (zonestats != NULL) {
isc_stats_detach(&zonestats);
}
if (rcvquerystats != NULL) {
dns_stats_detach(&rcvquerystats);
}
if (dnssecsignstats != NULL) {
dns_stats_detach(&dnssecsignstats);
}
isc_log_write(NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_SERVER,
ISC_LOG_ERROR, "Failed at zone_xmlrender()");
return ISC_R_FAILURE;
Expand Down Expand Up @@ -2343,6 +2355,9 @@ zone_jsonrender(dns_zone_t *zone, void *arg) {
json_object *zoneobj = NULL;
dns_zonestat_level_t statlevel;
isc_time_t timestamp;
isc_stats_t *zonestats = NULL;
dns_stats_t *rcvquerystats = NULL;
dns_stats_t *dnssecsignstats = NULL;

statlevel = dns_zone_getstatlevel(zone);
if (statlevel == dns_zonestat_none) {
Expand Down Expand Up @@ -2392,14 +2407,11 @@ zone_jsonrender(dns_zone_t *zone, void *arg) {
}

if (statlevel == dns_zonestat_full) {
isc_stats_t *zonestats;
isc_stats_t *gluecachestats;
dns_stats_t *rcvquerystats;
dns_stats_t *dnssecsignstats;
uint64_t nsstat_values[ns_statscounter_max];
uint64_t gluecachestats_values[dns_gluecachestatscounter_max];

zonestats = dns_zone_getrequeststats(zone);
dns_zone_getrequeststats(zone, &zonestats);
if (zonestats != NULL) {
json_object *counters = json_object_new_object();
if (counters == NULL) {
Expand Down Expand Up @@ -2450,7 +2462,7 @@ zone_jsonrender(dns_zone_t *zone, void *arg) {
}
}

rcvquerystats = dns_zone_getrcvquerystats(zone);
dns_zone_getrcvquerystats(zone, &rcvquerystats);
if (rcvquerystats != NULL) {
stats_dumparg_t dumparg;
json_object *counters = json_object_new_object();
Expand All @@ -2474,7 +2486,7 @@ zone_jsonrender(dns_zone_t *zone, void *arg) {
}
}

dnssecsignstats = dns_zone_getdnssecsignstats(zone);
dns_zone_getdnssecsignstats(zone, &dnssecsignstats);
if (dnssecsignstats != NULL) {
stats_dumparg_t dumparg;
json_object *sign_counters = json_object_new_object();
Expand Down Expand Up @@ -2530,6 +2542,15 @@ zone_jsonrender(dns_zone_t *zone, void *arg) {
result = ISC_R_SUCCESS;

cleanup:
if (zonestats != NULL) {
isc_stats_detach(&zonestats);
}
if (rcvquerystats != NULL) {
dns_stats_detach(&rcvquerystats);
}
if (dnssecsignstats != NULL) {
dns_stats_detach(&dnssecsignstats);
}
if (zoneobj != NULL) {
json_object_put(zoneobj);
}
Expand Down Expand Up @@ -4102,12 +4123,14 @@ named_stats_dump(named_server_t *server, FILE *fp) {
result == ISC_R_SUCCESS;
next = NULL, result = dns_zone_next(zone, &next), zone = next)
{
isc_stats_t *zonestats = dns_zone_getrequeststats(zone);
isc_stats_t *zonestats = NULL;
dns_zone_getrequeststats(zone, &zonestats);
if (zonestats != NULL) {
char zonename[DNS_NAME_FORMATSIZE];

view = dns_zone_getview(zone);
if (view == NULL) {
isc_stats_detach(&zonestats);
continue;
}

Expand All @@ -4123,6 +4146,7 @@ named_stats_dump(named_server_t *server, FILE *fp) {
NULL, nsstats_desc,
ns_statscounter_max, nsstats_index,
nsstat_values, 0);
isc_stats_detach(&zonestats);
}
}

Expand Down
13 changes: 7 additions & 6 deletions lib/dns/include/dns/zone.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2076,19 +2076,20 @@ dns_zone_setdnssecsignstats(dns_zone_t *zone, dns_stats_t *stats);
*\li stats is a valid statistics.
*/

isc_stats_t *
dns_zone_getrequeststats(dns_zone_t *zone);
void
dns_zone_getrequeststats(dns_zone_t *zone, isc_stats_t **statsp);

dns_stats_t *
dns_zone_getrcvquerystats(dns_zone_t *zone);
void
dns_zone_getrcvquerystats(dns_zone_t *zone, dns_stats_t **statsp);

dns_stats_t *
dns_zone_getdnssecsignstats(dns_zone_t *zone);
void
dns_zone_getdnssecsignstats(dns_zone_t *zone, dns_stats_t **statsp);
/*%<
* Get the additional statistics for zone, if one is installed.
*
* Requires:
* \li 'zone' to be a valid zone.
* \li 'statsp' to be non-NULL and *stastp to be NULL.
*
* Returns:
* \li when available, a pointer to the statistics set installed in zone;
Expand Down
7 changes: 6 additions & 1 deletion lib/dns/update.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1107,7 +1107,7 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
dns_kasp_t *kasp = dns_zone_getkasp(zone);
dns_rdataset_t rdataset;
dns_rdata_t sig_rdata = DNS_RDATA_INIT;
dns_stats_t *dnssecsignstats = dns_zone_getdnssecsignstats(zone);
dns_stats_t *dnssecsignstats = NULL;
isc_buffer_t buffer;
unsigned char data[1024]; /* XXX */
unsigned int i;
Expand All @@ -1121,6 +1121,8 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
offlineksk = dns_kasp_offlineksk(kasp);
}

dns_zone_getdnssecsignstats(zone, &dnssecsignstats);

dns_rdataset_init(&rdataset);
isc_buffer_init(&buffer, data, sizeof(data));

Expand Down Expand Up @@ -1276,6 +1278,9 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
}

failure:
if (dnssecsignstats != NULL) {
dns_stats_detach(&dnssecsignstats);
}
if (dns_rdataset_isassociated(&rdataset)) {
dns_rdataset_disassociate(&rdataset);
}
Expand Down
64 changes: 43 additions & 21 deletions lib/dns/zone.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ typedef struct dns_include dns_include_t;
} while (0)
#define UNLOCK_ZONE(z) \
do { \
INSIST((z)->locked); \
(z)->locked = false; \
UNLOCK(&(z)->lock); \
} while (0)
Expand Down Expand Up @@ -6858,7 +6859,7 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
isc_stdtime_t inception, isc_stdtime_t expire) {
isc_result_t result;
dns_dbnode_t *node = NULL;
dns_stats_t *dnssecsignstats;
dns_stats_t *dnssecsignstats = NULL;
dns_rdataset_t rdataset;
dns_rdata_t sig_rdata = DNS_RDATA_INIT;
unsigned char data[1024]; /* XXX */
Expand Down Expand Up @@ -7033,7 +7034,7 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
isc_buffer_init(&buffer, data, sizeof(data));

/* Update DNSSEC sign statistics. */
dnssecsignstats = dns_zone_getdnssecsignstats(zone);
dns_zone_getdnssecsignstats(zone, &dnssecsignstats);
if (dnssecsignstats != NULL) {
/* Generated a new signature. */
dns_dnssecsignstats_increment(dnssecsignstats,
Expand All @@ -7045,6 +7046,7 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
dnssecsignstats, ID(keys[i]),
(uint8_t)ALG(keys[i]),
dns_dnssecsignstats_refresh);
dns_stats_detach(&dnssecsignstats);
}
}

Expand Down Expand Up @@ -7516,7 +7518,7 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name,
dns_rdatasetiter_t *iterator = NULL;
dns_rdataset_t rdataset;
dns_rdata_t rdata = DNS_RDATA_INIT;
dns_stats_t *dnssecsignstats;
dns_stats_t *dnssecsignstats = NULL;
bool offlineksk = false;
isc_buffer_t buffer;
unsigned char data[1024];
Expand Down Expand Up @@ -7649,7 +7651,7 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name,
dns_rdata_reset(&rdata);

/* Update DNSSEC sign statistics. */
dnssecsignstats = dns_zone_getdnssecsignstats(zone);
dns_zone_getdnssecsignstats(zone, &dnssecsignstats);
if (dnssecsignstats != NULL) {
/* Generated a new signature. */
dns_dnssecsignstats_increment(dnssecsignstats, ID(key),
Expand All @@ -7659,6 +7661,7 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name,
dns_dnssecsignstats_increment(
dnssecsignstats, ID(key), ALG(key),
dns_dnssecsignstats_refresh);
dns_stats_detach(&dnssecsignstats);
}

(*signatures)--;
Expand Down Expand Up @@ -19774,15 +19777,27 @@ dns_zone_setdnssecsignstats(dns_zone_t *zone, dns_stats_t *stats) {
UNLOCK_ZONE(zone);
}

dns_stats_t *
dns_zone_getdnssecsignstats(dns_zone_t *zone) {
static void
getdnssecsignstats(dns_zone_t *zone, dns_stats_t **statsp) {
REQUIRE(statsp != NULL && *statsp == NULL);
if (zone->dnssecsignstats != NULL) {
dns_stats_attach(zone->dnssecsignstats, statsp);
}
}
void
dns_zone_getdnssecsignstats(dns_zone_t *zone, dns_stats_t **statsp) {
REQUIRE(DNS_ZONE_VALID(zone));

return zone->dnssecsignstats;
LOCK_ZONE(zone);
getdnssecsignstats(zone, statsp);
UNLOCK_ZONE(zone);
}

isc_stats_t *
dns_zone_getrequeststats(dns_zone_t *zone) {
void
dns_zone_getrequeststats(dns_zone_t *zone, isc_stats_t **statsp) {
REQUIRE(DNS_ZONE_VALID(zone));
REQUIRE(statsp != NULL && *statsp == NULL);

/*
* We don't lock zone for efficiency reason. This is not catastrophic
* because requeststats must always be valid when requeststats_on is
Expand All @@ -19791,24 +19806,27 @@ dns_zone_getrequeststats(dns_zone_t *zone) {
* false, or some cannot be incremented just after the statistics are
* installed, but it shouldn't matter much in practice.
*/
if (zone->requeststats_on) {
return zone->requeststats;
} else {
return NULL;
LOCK_ZONE(zone);
if (zone->requeststats_on && zone->requeststats != NULL) {
isc_stats_attach(zone->requeststats, statsp);
}
UNLOCK_ZONE(zone);
}

/*
* Return the received query stats bucket
* see note from dns_zone_getrequeststats()
*/
dns_stats_t *
dns_zone_getrcvquerystats(dns_zone_t *zone) {
if (zone->requeststats_on) {
return zone->rcvquerystats;
} else {
return NULL;
void
dns_zone_getrcvquerystats(dns_zone_t *zone, dns_stats_t **statsp) {
REQUIRE(DNS_ZONE_VALID(zone));
REQUIRE(statsp != NULL && *statsp == NULL);

LOCK_ZONE(zone);
if (zone->requeststats_on && zone->rcvquerystats != NULL) {
dns_stats_attach(zone->rcvquerystats, statsp);
}
UNLOCK_ZONE(zone);
}

isc_result_t
Expand Down Expand Up @@ -22617,8 +22635,9 @@ zone_rekey(dns_zone_t *zone) {

if (commit) {
dns_difftuple_t *tuple;
dns_stats_t *dnssecsignstats =
dns_zone_getdnssecsignstats(zone);
dns_stats_t *dnssecsignstats = NULL;

getdnssecsignstats(zone, &dnssecsignstats);

DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NEEDNOTIFY);

Expand Down Expand Up @@ -22658,6 +22677,9 @@ zone_rekey(dns_zone_t *zone) {
}
}
}
if (dnssecsignstats != NULL) {
dns_stats_detach(&dnssecsignstats);
}

if (fullsign) {
/*
Expand Down
Loading

0 comments on commit 3c720c6

Please sign in to comment.