From: Ondřej Surý Date: Tue, 16 Dec 2025 10:11:05 +0000 (+0100) Subject: Copy only the raw data when we are copying dns_slab{header,vec} X-Git-Tag: v9.21.17~33^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=62cb5b30da616b3a3e960afff1952024361c17a3;p=thirdparty%2Fbind9.git Copy only the raw data when we are copying dns_slab{header,vec} The makeslab function in rdataslab.c contains an optimization for cases where the source is already an rdataslab. In these cases, it copies the entire slab using memmove. However, this creates a race condition: while the target slab is protected by a node lock, the source slab is not protected. This becomes problematic because the TTL heap needs to modify the heap index stored in the slab header, potentially while the memmove operation is reading from it. A closer look at makeslab shows that copying the header part of the slab is unnecessary, the header can be default-initialized instead. This MR modifies makeslab to copy only the raw part of the slab, while default-initializing the header, eliminating the race condition. For consistency, it also applies the same change to vecheader/makevec. --- diff --git a/lib/dns/rdataslab.c b/lib/dns/rdataslab.c index 578e84ae502..48ac591eebd 100644 --- a/lib/dns/rdataslab.c +++ b/lib/dns/rdataslab.c @@ -17,6 +17,8 @@ #include #include +#include + #include #include #include @@ -100,6 +102,24 @@ compare_rdata(const void *p1, const void *p2) { return dns_rdata_compare(p1, p2); } +static unsigned char * +newslab(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, + size_t size) { + dns_slabheader_t *header = isc_mem_get(mctx, size); + + *header = (dns_slabheader_t){ + .headers_link = CDS_LIST_HEAD_INIT(header->headers_link), + .trust = rdataset->trust, + .ttl = rdataset->ttl, + .dirtylink = ISC_LINK_INITIALIZER, + }; + + region->base = (unsigned char *)header; + region->length = size; + + return (unsigned char *)header + sizeof(*header); +} + static isc_result_t makeslab(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, uint32_t maxrrperset) { @@ -128,11 +148,11 @@ makeslab(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, dns_slabheader_t *header = rdataset_getheader(rdataset); buflen = dns_rdataslab_size(header); - rawbuf = isc_mem_get(mctx, buflen); - region->base = rawbuf; - region->length = buflen; + rawbuf = newslab(rdataset, mctx, region, buflen); - memmove(rawbuf, header, buflen); + INSIST(headerlen <= buflen); + memmove(rawbuf, (unsigned char *)header + headerlen, + buflen - headerlen); return ISC_R_SUCCESS; } @@ -145,10 +165,7 @@ makeslab(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, if (rdataset->type != 0) { return ISC_R_FAILURE; } - rawbuf = isc_mem_get(mctx, buflen); - region->base = rawbuf; - region->length = buflen; - rawbuf += headerlen; + rawbuf = newslab(rdataset, mctx, region, buflen); put_uint16(rawbuf, 0); return ISC_R_SUCCESS; } @@ -253,11 +270,7 @@ makeslab(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, * Allocate the memory, set up a buffer, start copying in * data. */ - rawbuf = isc_mem_get(mctx, buflen); - - region->base = rawbuf; - region->length = buflen; - rawbuf += headerlen; + rawbuf = newslab(rdataset, mctx, region, buflen); put_uint16(rawbuf, nitems); for (i = 0; i < nalloc; i++) { @@ -306,29 +319,20 @@ dns_rdataslab_fromrdataset(dns_rdataset_t *rdataset, isc_mem_t *mctx, result = makeslab(rdataset, mctx, region, maxrrperset); if (result == ISC_R_SUCCESS) { - dns_slabheader_t *new = (dns_slabheader_t *)region->base; - dns_typepair_t typepair; + dns_slabheader_t *header = (dns_slabheader_t *)region->base; if (rdataset->attributes.negative) { INSIST(rdataset->type == dns_rdatatype_none); INSIST(rdataset->covers != dns_rdatatype_none); - typepair = DNS_TYPEPAIR_VALUE(rdataset->covers, - dns_rdatatype_none); + header->typepair = DNS_TYPEPAIR_VALUE( + rdataset->covers, dns_rdatatype_none); } else { INSIST(rdataset->type != dns_rdatatype_none); INSIST(dns_rdatatype_issig(rdataset->type) || rdataset->covers == dns_rdatatype_none); - typepair = DNS_TYPEPAIR_VALUE(rdataset->type, - rdataset->covers); + header->typepair = DNS_TYPEPAIR_VALUE(rdataset->type, + rdataset->covers); } - - *new = (dns_slabheader_t){ - .headers_link = CDS_LIST_HEAD_INIT(new->headers_link), - .typepair = typepair, - .trust = rdataset->trust, - .ttl = rdataset->ttl, - .dirtylink = ISC_LINK_INITIALIZER, - }; } return result; diff --git a/lib/dns/rdatavec.c b/lib/dns/rdatavec.c index 9b69bf5e6e3..c4672caa4e9 100644 --- a/lib/dns/rdatavec.c +++ b/lib/dns/rdatavec.c @@ -123,6 +123,23 @@ rdatavec_count(dns_vecheader_t *header) { return count; } +static unsigned char * +newvec(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, + size_t size) { + dns_vecheader_t *header = isc_mem_get(mctx, size); + + *header = (dns_vecheader_t){ + .next_header = ISC_SLINK_INITIALIZER, + .trust = rdataset->trust, + .ttl = rdataset->ttl, + }; + + region->base = (unsigned char *)header; + region->length = size; + + return (unsigned char *)header + sizeof(*header); +} + static isc_result_t makevec(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, uint32_t maxrrperset) { @@ -151,11 +168,11 @@ makevec(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, dns_vecheader_t *header = dns_vecheader_getheader(rdataset); buflen = dns_rdatavec_size(header); - rawbuf = isc_mem_get(mctx, buflen); - region->base = rawbuf; - region->length = buflen; + rawbuf = newvec(rdataset, mctx, region, buflen); - memmove(rawbuf, header, buflen); + INSIST(headerlen <= buflen); + memmove(rawbuf, (unsigned char *)header + headerlen, + buflen - headerlen); return ISC_R_SUCCESS; } @@ -168,10 +185,7 @@ makevec(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, if (rdataset->type != 0) { return ISC_R_FAILURE; } - rawbuf = isc_mem_get(mctx, buflen); - region->base = rawbuf; - region->length = buflen; - rawbuf += headerlen; + rawbuf = newvec(rdataset, mctx, region, buflen); put_uint16(rawbuf, 0); return ISC_R_SUCCESS; } @@ -276,11 +290,7 @@ makevec(dns_rdataset_t *rdataset, isc_mem_t *mctx, isc_region_t *region, * Allocate the memory, set up a buffer, start copying in * data. */ - rawbuf = isc_mem_get(mctx, buflen); - - region->base = rawbuf; - region->length = buflen; - rawbuf += headerlen; + rawbuf = newvec(rdataset, mctx, region, buflen); put_uint16(rawbuf, nitems); for (i = 0; i < nalloc; i++) {