Use dns_name_copynf() with dns_message_gettempname() when needed

dns_message_gettempname() returns an initialized name with a dedicated
buffer, associated with a dns_fixedname object.  Using dns_name_copynf()
to write a name into this object will actually copy the name data
from a source name. dns_name_clone() merely points target->ndata to
source->ndata, so it is faster, but it can lead to a use-after-free if
the source is freed before the target object is released via
dns_message_puttempname().

In a few places, clone was being used where copynf should have been;
this is now fixed.

As a side note, no memory was lost, because the ndata buffer used in
the dns_fixedname_t is internal to the structure, and is freed when
the dns_fixedname_t is freed regardless of the .ndata contents.

lib/dns/include/dns/name.h

@@ -638,7 +638,7 @@ dns_name_clone(const dns_name_t *source, dns_name_t *target);
  * Notes:
  *
  * \li	'target' refers to the same memory as 'source', so 'source'
- *	must not be changed while 'target' is still in use.
+ *	must not be changed or freed while 'target' is still in use.
  *
  * \li	This call is functionally equivalent to:
  *
@@ -1242,22 +1242,17 @@ dns_name_settotextfilter(dns_name_totextfilter_t *proc);
 
 isc_result_t
 dns_name_copy(const dns_name_t *source, dns_name_t *dest, isc_buffer_t *target);
-void
-dns_name_copynf(const dns_name_t *source, dns_name_t *dest);
 /*%<
- * Makes 'dest' refer to a copy of the name in 'source'.  The data are either
- * copied to 'target' or in case of dns_name_copynf the dedicated buffer in
- * 'dest'.
+ * Copies the name in 'source' into 'dest'.  The name data is copied to
+ * the 'target' buffer, which is then set as the buffer for 'dest'.
  *
  * Requires:
  * \li	'source' is a valid name.
  *
- * \li	'dest' is an initialized name with a dedicated buffer.
+ * \li	'dest' is an initialized name.
  *
  * \li	'target' is an initialized buffer.
  *
- * \li	Either dest has a dedicated buffer or target != NULL.
- *
  * Ensures:
  *
  *\li	On success, the used space in target is updated.
@@ -1267,6 +1262,18 @@ dns_name_copynf(const dns_name_t *source, dns_name_t *dest);
  *\li	#ISC_R_NOSPACE
  */
 
+void
+dns_name_copynf(const dns_name_t *source, dns_name_t *dest);
+/*%<
+ * Copies the name in 'source' into 'dest'.  The name data is copied to
+ * the dedicated buffer for 'dest'.
+ *
+ * Requires:
+ * \li	'source' is a valid name.
+ *
+ * \li	'dest' is an initialized name with a dedicated buffer.
+ */
+
 bool
 dns_name_ishostname(const dns_name_t *name, bool wildcard);
 /*%<

lib/dns/name.c

@@ -2464,7 +2464,7 @@ dns_name_fromstring2(dns_name_t *target, const char *src,
 
 static isc_result_t
 name_copy(const dns_name_t *source, dns_name_t *dest, isc_buffer_t *target) {
-	unsigned char *ndata;
+	unsigned char *ndata = NULL;
 
 	/*
 	 * Make dest a copy of source.
@@ -2496,7 +2496,7 @@ name_copy(const dns_name_t *source, dns_name_t *dest, isc_buffer_t *target) {
 	}
 
 	if (dest->labels > 0 && dest->offsets != NULL) {
-		if (source->offsets != NULL) {
+		if (source->offsets != NULL && source->labels != 0) {
 			memmove(dest->offsets, source->offsets, source->labels);
 		} else {
 			set_offsets(dest, dest->offsets, NULL);

lib/dns/tkey.c

@@ -180,7 +180,7 @@ add_rdata_to_list(dns_message_t *msg, dns_name_t *name, dns_rdata_t *rdata,
 	dns_message_takebuffer(msg, &tmprdatabuf);
 
 	RETERR(dns_message_gettempname(msg, &newname));
-	dns_name_clone(name, newname);
+	dns_name_copynf(name, newname);
 
 	RETERR(dns_message_gettemprdatalist(msg, &newlist));
 	newlist->rdclass = newrdata->rdclass;

lib/dns/tsig.c

@@ -1020,7 +1020,7 @@ dns_tsig_sign(dns_message_t *msg) {
 	if (ret != ISC_R_SUCCESS) {
 		goto cleanup_rdata;
 	}
-	dns_name_clone(&key->name, owner);
+	dns_name_copynf(&key->name, owner);
 
 	ret = dns_message_gettemprdatalist(msg, &datalist);
 	if (ret != ISC_R_SUCCESS) {

lib/ns/query.c

@@ -9804,7 +9804,7 @@ query_synthcnamewildcard(query_ctx_t *qctx, dns_rdataset_t *rdataset,
 	RUNTIME_CHECK(result == ISC_R_SUCCESS);
 	dns_rdata_reset(&rdata);
 
-	dns_name_clone(&cname.cname, tname);
+	dns_name_copynf(&cname.cname, tname);
 
 	dns_rdata_freestruct(&cname);
 	ns_client_qnamereplace(qctx->client, tname);
@@ -10416,7 +10416,7 @@ query_cname(query_ctx_t *qctx) {
 	RUNTIME_CHECK(result == ISC_R_SUCCESS);
 	dns_rdata_reset(&rdata);
 
-	dns_name_clone(&cname.cname, tname);
+	dns_name_copynf(&cname.cname, tname);
 
 	dns_rdata_freestruct(&cname);
 	ns_client_qnamereplace(qctx->client, tname);
@@ -10518,7 +10518,7 @@ query_dname(query_ctx_t *qctx) {
 	RUNTIME_CHECK(result == ISC_R_SUCCESS);
 	dns_rdata_reset(&rdata);
 
-	dns_name_clone(&dname.dname, tname);
+	dns_name_copynf(&dname.dname, tname);
 	dns_rdata_freestruct(&dname);
 
 	/*
@@ -10616,7 +10616,8 @@ query_addcname(query_ctx_t *qctx, dns_trust_t trust, dns_ttl_t ttl) {
 	if (result != ISC_R_SUCCESS) {
 		return (result);
 	}
-	dns_name_clone(client->query.qname, aname);
+
+	dns_name_copynf(client->query.qname, aname);
 
 	result = dns_message_gettemprdatalist(client->message, &rdatalist);
 	if (result != ISC_R_SUCCESS) {
@@ -10751,7 +10752,13 @@ query_addsoa(query_ctx_t *qctx, unsigned int override_ttl,
 	if (result != ISC_R_SUCCESS) {
 		return (result);
 	}
+
+	/*
+	 * We'll be releasing 'name' before returning, so it's safe to
+	 * use clone instead of copying here.
+	 */
 	dns_name_clone(dns_db_origin(qctx->db), name);
+
 	rdataset = ns_client_newrdataset(client);
 	if (rdataset == NULL) {
 		CTRACE(ISC_LOG_ERROR, "unable to allocate rdataset");