From 2cf0fe3b8092f64f8f68ae3693fe2e73e90ad1a4 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Mon, 22 Jan 2018 09:36:12 +1100 Subject: [PATCH] 4869. [bug] Address some cases where NULL with zero length could be passed to memmove which is undefined behaviour and can lead to bad optimisation. [RT #46888] (cherry picked from commit fdd8838bf9c4de07372196607f860dd240986577) --- CHANGES | 4 ++++ lib/dns/catz.c | 15 ++++++++------- lib/dns/diff.c | 11 ++++++++--- lib/dns/ipkeylist.c | 24 ++++++++++++++++-------- lib/dns/tests/dispatch_test.c | 4 ++-- 5 files changed, 38 insertions(+), 20 deletions(-) diff --git a/CHANGES b/CHANGES index 2285bfb690..7fb9a321d4 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +4869. [bug] Address some cases where NULL with zero length could + be passed to memmove which is undefined behaviour and + can lead to bad optimisation. [RT #46888] + 4867. [cleanup] Normalize rndc on/off commands (validation and querylog) so they accept the same synonyms for on/off (yes/no, true/false, enable/disable). diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 69f862d3ad..049907b12e 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -1002,7 +1002,7 @@ catz_process_version(dns_catz_zone_t *zone, dns_rdataset_t *value) { result = ISC_R_BADNUMBER; goto cleanup; } - memcpy(t, rdatastr.data, rdatastr.length); + memmove(t, rdatastr.data, rdatastr.length); t[rdatastr.length] = 0; result = isc_parse_uint32(&tversion, t, 10); if (result != ISC_R_SUCCESS) { @@ -1097,7 +1097,7 @@ catz_process_masters(dns_catz_zone_t *zone, dns_ipkeylist_t *ipkl, if (keyname == NULL) return (ISC_R_NOMEMORY); dns_name_init(keyname, 0); - memcpy(keycbuf, rdatastr.data, rdatastr.length); + memmove(keycbuf, rdatastr.data, rdatastr.length); keycbuf[rdatastr.length] = 0; result = dns_name_fromstring(keyname, keycbuf, 0, mctx); if (result != ISC_R_SUCCESS) { @@ -1126,8 +1126,8 @@ catz_process_masters(dns_catz_zone_t *zone, dns_ipkeylist_t *ipkl, if (value->type == dns_rdatatype_txt) ipkl->keys[i] = keyname; else /* A/AAAA */ - memcpy(&ipkl->addrs[i], &sockaddr, - sizeof(isc_sockaddr_t)); + memmove(&ipkl->addrs[i], &sockaddr, + sizeof(isc_sockaddr_t)); } else { result = dns_ipkeylist_resize(mctx, ipkl, i+1); @@ -1158,8 +1158,8 @@ catz_process_masters(dns_catz_zone_t *zone, dns_ipkeylist_t *ipkl, if (value->type == dns_rdatatype_txt) ipkl->keys[i] = keyname; else /* A/AAAA */ - memcpy(&ipkl->addrs[i], &sockaddr, - sizeof(isc_sockaddr_t)); + memmove(&ipkl->addrs[i], &sockaddr, + sizeof(isc_sockaddr_t)); ipkl->count++; } return (ISC_R_SUCCESS); @@ -1251,7 +1251,8 @@ catz_process_apl(dns_catz_zone_t *zone, isc_buffer_t **aclbp, result = dns_rdata_apl_current(&rdata_apl, &apl_ent); RUNTIME_CHECK(result == ISC_R_SUCCESS); memset(buf, 0, sizeof(buf)); - memcpy(buf, apl_ent.data, apl_ent.length); + if (apl_ent.data != NULL && apl_ent.length > 0) + memmove(buf, apl_ent.data, apl_ent.length); if (apl_ent.family == 1) isc_netaddr_fromin(&addr, (struct in_addr*) buf); else if (apl_ent.family == 2) diff --git a/lib/dns/diff.c b/lib/dns/diff.c index eb5e21d188..47fa026285 100644 --- a/lib/dns/diff.c +++ b/lib/dns/diff.c @@ -80,11 +80,16 @@ dns_difftuple_create(isc_mem_t *mctx, t->ttl = ttl; - memmove(datap, rdata->data, rdata->length); dns_rdata_init(&t->rdata); dns_rdata_clone(rdata, &t->rdata); - t->rdata.data = datap; - datap += rdata->length; + if (rdata->data != NULL) { + memmove(datap, rdata->data, rdata->length); + t->rdata.data = datap; + datap += rdata->length; + } else { + t->rdata.data = NULL; + INSIST(rdata->length == 0); + } ISC_LINK_INIT(&t->rdata, link); ISC_LINK_INIT(t, link); diff --git a/lib/dns/ipkeylist.c b/lib/dns/ipkeylist.c index dadcfd6df7..be6d8a7b88 100644 --- a/lib/dns/ipkeylist.c +++ b/lib/dns/ipkeylist.c @@ -187,34 +187,42 @@ dns_ipkeylist_resize(isc_mem_t *mctx, dns_ipkeylist_t *ipkl, unsigned int n) { if (labels == NULL) goto nomemory; - memmove(addrs, ipkl->addrs, ipkl->allocated * sizeof(isc_sockaddr_t)); - if (ipkl->addrs != NULL) + if (ipkl->addrs != NULL) { + memmove(addrs, ipkl->addrs, + ipkl->allocated * sizeof(isc_sockaddr_t)); isc_mem_put(mctx, ipkl->addrs, ipkl->allocated * sizeof(isc_sockaddr_t)); + } ipkl->addrs = addrs; memset(&ipkl->addrs[ipkl->allocated], 0, (n - ipkl->allocated) * sizeof(isc_sockaddr_t)); - memmove(dscps, ipkl->dscps, ipkl->allocated * sizeof(isc_dscp_t)); - if (ipkl->dscps != NULL) + if (ipkl->dscps != NULL) { + memmove(dscps, ipkl->dscps, + ipkl->allocated * sizeof(isc_dscp_t)); isc_mem_put(mctx, ipkl->dscps, ipkl->allocated * sizeof(isc_dscp_t)); + } ipkl->dscps = dscps; memset(&ipkl->dscps[ipkl->allocated], 0, (n - ipkl->allocated) * sizeof(isc_dscp_t)); - memmove(keys, ipkl->keys, ipkl->allocated * sizeof(dns_name_t *)); - if (ipkl->keys) + if (ipkl->keys) { + memmove(keys, ipkl->keys, + ipkl->allocated * sizeof(dns_name_t *)); isc_mem_put(mctx, ipkl->keys, ipkl->allocated * sizeof(dns_name_t *)); + } ipkl->keys = keys; memset(&ipkl->keys[ipkl->allocated], 0, (n - ipkl->allocated) * sizeof(dns_name_t *)); - memmove(labels, ipkl->labels, ipkl->allocated * sizeof(dns_name_t *)); - if (ipkl->labels) + if (ipkl->labels != NULL) { + memmove(labels, ipkl->labels, + ipkl->allocated * sizeof(dns_name_t *)); isc_mem_put(mctx, ipkl->labels, ipkl->allocated * sizeof(dns_name_t *)); + } ipkl->labels = labels; memset(&ipkl->labels[ipkl->allocated], 0, (n - ipkl->allocated) * sizeof(dns_name_t *)); diff --git a/lib/dns/tests/dispatch_test.c b/lib/dns/tests/dispatch_test.c index 9627e7e065..0b40256f86 100644 --- a/lib/dns/tests/dispatch_test.c +++ b/lib/dns/tests/dispatch_test.c @@ -157,11 +157,11 @@ nameserver(isc_task_t *task, isc_event_t *event) { static unsigned char buf1[16]; static unsigned char buf2[16]; - memcpy(buf1, ev->region.base, 12); + memmove(buf1, ev->region.base, 12); memset(buf1 + 12, 0, 4); buf1[2] |= 0x80; /* qr=1 */ - memcpy(buf2, ev->region.base, 12); + memmove(buf2, ev->region.base, 12); memset(buf2 + 12, 1, 4); buf2[2] |= 0x80; /* qr=1 */