Merge branch '3655-decompress-faster' into 'main'

Simplify and speed up DNS name decompression

Closes #3655

See merge request isc-projects/bind9!7045

CHANGES

@@ -1,3 +1,6 @@
+6022.	[performance]	The decompression implementation in dns_name_fromwire()
+			is now smaller and faster. [GL #3655]
+
 6021.	[bug]		Use the current domain name when checking answers from
 			a dual-stack-server. [GL #3607]
 

fuzz/.gitignore

@@ -4,6 +4,7 @@
 /dns_message_checksig
 /dns_message_parse
 /dns_name_fromtext_target
+/dns_name_fromwire
 /dns_rdata_fromtext
 /dns_rdata_fromwire_text
 /isc_lex_getmastertoken

fuzz/Makefile.am

@@ -26,6 +26,7 @@ check_PROGRAMS =			\
 	dns_message_checksig		\
 	dns_message_parse		\
 	dns_name_fromtext_target	\
+	dns_name_fromwire		\
 	dns_rdata_fromtext		\
 	dns_rdata_fromwire_text		\
 	isc_lex_getmastertoken		\
@@ -36,11 +37,17 @@ EXTRA_DIST =				\
 	dns_message_checksig.in		\
 	dns_message_parse.in		\
 	dns_name_fromtext_target.in	\
+	dns_name_fromwire.in		\
 	dns_rdata_fromtext.in		\
 	dns_rdata_fromwire_text.in	\
 	isc_lex_getmastertoken.in	\
 	isc_lex_gettoken.in
 
+dns_name_fromwire_SOURCES =		\
+	dns_name_fromwire.c		\
+	old.c				\
+	old.h
+
 TESTS = $(check_PROGRAMS)
 
 if HAVE_FUZZ_LOG_COMPILER

fuzz/dns_name_fromwire.c

@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+#include <stddef.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <isc/ascii.h>
+#include <isc/buffer.h>
+#include <isc/util.h>
+
+#include <dns/compress.h>
+#include <dns/fixedname.h>
+#include <dns/name.h>
+
+#include "fuzz.h"
+#include "old.h"
+
+bool debug = false;
+
+int
+LLVMFuzzerInitialize(int *argc __attribute__((unused)),
+		     char ***argv __attribute__((unused))) {
+	return (0);
+}
+
+int
+LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
+	isc_result_t new_result;
+	isc_result_t old_result;
+	dns_fixedname_t new_fixed;
+	dns_fixedname_t old_fixed;
+	dns_name_t *new_name = dns_fixedname_initname(&new_fixed);
+	dns_name_t *old_name = dns_fixedname_initname(&old_fixed);
+	uint8_t *new_offsets;
+	uint8_t *old_offsets;
+	dns_decompress_t dctx = DNS_DECOMPRESS_PERMITTED;
+	isc_buffer_t new_buf;
+	isc_buffer_t old_buf;
+
+	/*
+	 * Output buffers may be partially used or undersized.
+	 */
+	if (size > 0) {
+		uint8_t add = *data++;
+		size--;
+		isc_buffer_add(&new_fixed.buffer, add);
+		isc_buffer_add(&old_fixed.buffer, add);
+	}
+
+	/*
+	 * timeout faster if we hit a pointer loop
+	 */
+	alarm(1);
+
+	/*
+	 * We shift forward by half the input data to make an area
+	 * that pointers can refer back to.
+	 */
+
+	isc_buffer_constinit(&new_buf, data, size);
+	isc_buffer_add(&new_buf, size);
+	isc_buffer_setactive(&new_buf, size);
+	isc_buffer_forward(&new_buf, size / 2);
+	new_result = dns_name_fromwire(new_name, &new_buf, dctx, 0, NULL);
+
+	isc_buffer_constinit(&old_buf, data, size);
+	isc_buffer_add(&old_buf, size);
+	isc_buffer_setactive(&old_buf, size);
+	isc_buffer_forward(&old_buf, size / 2);
+	old_result = old_name_fromwire(old_name, &old_buf, dctx, 0, NULL);
+
+	REQUIRE(new_result == old_result);
+	REQUIRE(dns_name_equal(new_name, old_name));
+	REQUIRE(new_name->labels == old_name->labels);
+
+	new_offsets = new_name->offsets;
+	old_offsets = old_name->offsets;
+	REQUIRE(new_offsets != NULL && old_offsets != NULL);
+	REQUIRE(memcmp(new_offsets, old_offsets, old_name->labels) == 0);
+
+	REQUIRE(new_fixed.buffer.current == old_fixed.buffer.current);
+	REQUIRE(new_fixed.buffer.active == old_fixed.buffer.active);
+	REQUIRE(new_fixed.buffer.used == old_fixed.buffer.used);
+	REQUIRE(new_fixed.buffer.length == old_fixed.buffer.length);
+
+	REQUIRE(new_buf.base == old_buf.base);
+	REQUIRE(new_buf.current == old_buf.current);
+	REQUIRE(new_buf.active == old_buf.active);
+	REQUIRE(new_buf.used == old_buf.used);
+	REQUIRE(new_buf.length == old_buf.length);
+
+	return (0);
+}

fuzz/dns_name_fromwire.in/608fa82ab128dfb43c9a064bfab677d9b5f3f72b

@@ -0,0 +1 @@
+<EFBFBD>0

fuzz/dns_name_fromwire.in/7c20e8ac688f92b55099a7f63ff2a4f4bcd7704a

@@ -0,0 +1 @@
+<EFBFBD>0<EFBFBD>w

fuzz/dns_name_fromwire.in/93dccf807dc4df3bfc1711ef3ad3bbd65a7d8c67

@@ -0,0 +1 @@
+0<EFBFBD>

fuzz/dns_name_fromwire.in/9a279b27d765ca2eccc67e1fc8bafb0fa8702208

@@ -0,0 +1,2 @@
+<EFBFBD><EFBFBD>
+<EFBFBD>

fuzz/dns_name_fromwire.in/bf7b68e9cc0467f7bbbfe1dc0dfd7c3124b3695c

@@ -0,0 +1 @@
+<EFBFBD><EFBFBD>

fuzz/dns_name_fromwire.in/df58248c414f342c81e056b40bee12d17a08bf61

@@ -0,0 +1 @@
+*

fuzz/old.c

@@ -0,0 +1,223 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+#include <isc/ascii.h>
+#include <isc/buffer.h>
+#include <isc/result.h>
+#include <isc/types.h>
+#include <isc/util.h>
+
+#include <dns/compress.h>
+#include <dns/types.h>
+
+/*
+ */
+
+#include "old.h"
+
+/*
+ * code copied from lib/dns/name.c as of commit
+ * 6967973568fe80b03e1729259f8907ce8792be34
+ */
+
+typedef enum { fw_start = 0, fw_ordinary, fw_newcurrent } fw_state;
+
+#define VALID_NAME(n) ISC_MAGIC_VALID(n, DNS_NAME_MAGIC)
+
+#define INIT_OFFSETS(name, var, default_offsets) \
+	if ((name)->offsets != NULL)             \
+		var = (name)->offsets;           \
+	else                                     \
+		var = (default_offsets);
+
+#define MAKE_EMPTY(name)                           \
+	do {                                       \
+		name->ndata = NULL;                \
+		name->length = 0;                  \
+		name->labels = 0;                  \
+		name->attributes.absolute = false; \
+	} while (0)
+
+#define BINDABLE(name) (!name->attributes.readonly && !name->attributes.dynamic)
+
+isc_result_t
+old_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx,
+		  unsigned int options, isc_buffer_t *target) {
+	unsigned char *cdata, *ndata;
+	unsigned int cused; /* Bytes of compressed name data used */
+	unsigned int nused, labels, n, nmax;
+	unsigned int current, new_current, biggest_pointer;
+	bool done;
+	fw_state state = fw_start;
+	unsigned int c;
+	unsigned char *offsets;
+	dns_offsets_t odata;
+	bool downcase;
+	bool seen_pointer;
+
+	/*
+	 * Copy the possibly-compressed name at source into target,
+	 * decompressing it.  Loop prevention is performed by checking
+	 * the new pointer against biggest_pointer.
+	 */
+
+	REQUIRE(VALID_NAME(name));
+	REQUIRE((target != NULL && ISC_BUFFER_VALID(target)) ||
+		(target == NULL && ISC_BUFFER_VALID(name->buffer)));
+
+	downcase = ((options & DNS_NAME_DOWNCASE) != 0);
+
+	if (target == NULL && name->buffer != NULL) {
+		target = name->buffer;
+		isc_buffer_clear(target);
+	}
+
+	REQUIRE(BINDABLE(name));
+
+	INIT_OFFSETS(name, offsets, odata);
+
+	/*
+	 * Make 'name' empty in case of failure.
+	 */
+	MAKE_EMPTY(name);
+
+	/*
+	 * Initialize things to make the compiler happy; they're not required.
+	 */
+	n = 0;
+	new_current = 0;
+
+	/*
+	 * Set up.
+	 */
+	labels = 0;
+	done = false;
+
+	ndata = isc_buffer_used(target);
+	nused = 0;
+	seen_pointer = false;
+
+	/*
+	 * Find the maximum number of uncompressed target name
+	 * bytes we are willing to generate.  This is the smaller
+	 * of the available target buffer length and the
+	 * maximum legal domain name length (255).
+	 */
+	nmax = isc_buffer_availablelength(target);
+	if (nmax > DNS_NAME_MAXWIRE) {
+		nmax = DNS_NAME_MAXWIRE;
+	}
+
+	cdata = isc_buffer_current(source);
+	cused = 0;
+
+	current = source->current;
+	biggest_pointer = current;
+
+	/*
+	 * Note:  The following code is not optimized for speed, but
+	 * rather for correctness.  Speed will be addressed in the future.
+	 */
+
+	while (current < source->active && !done) {
+		c = *cdata++;
+		current++;
+		if (!seen_pointer) {
+			cused++;
+		}
+
+		switch (state) {
+		case fw_start:
+			if (c < 64) {
+				offsets[labels] = nused;
+				labels++;
+				if (nused + c + 1 > nmax) {
+					goto full;
+				}
+				nused += c + 1;
+				*ndata++ = c;
+				if (c == 0) {
+					done = true;
+				}
+				n = c;
+				state = fw_ordinary;
+			} else if (c >= 192) {
+				/*
+				 * 14-bit compression pointer
+				 */
+				if (!dns_decompress_getpermitted(dctx)) {
+					return (DNS_R_DISALLOWED);
+				}
+				new_current = c & 0x3F;
+				state = fw_newcurrent;
+			} else {
+				return (DNS_R_BADLABELTYPE);
+			}
+			break;
+		case fw_ordinary:
+			if (downcase) {
+				c = isc_ascii_tolower(c);
+			}
+			*ndata++ = c;
+			n--;
+			if (n == 0) {
+				state = fw_start;
+			}
+			break;
+		case fw_newcurrent:
+			new_current *= 256;
+			new_current += c;
+			if (new_current >= biggest_pointer) {
+				return (DNS_R_BADPOINTER);
+			}
+			biggest_pointer = new_current;
+			current = new_current;
+			cdata = (unsigned char *)source->base + current;
+			seen_pointer = true;
+			state = fw_start;
+			break;
+		default:
+			FATAL_ERROR("Unknown state %d", state);
+			/* Does not return. */
+		}
+	}
+
+	if (!done) {
+		return (ISC_R_UNEXPECTEDEND);
+	}
+
+	name->ndata = (unsigned char *)target->base + target->used;
+	name->labels = labels;
+	name->length = nused;
+	name->attributes.absolute = true;
+
+	isc_buffer_forward(source, cused);
+	isc_buffer_add(target, name->length);
+
+	return (ISC_R_SUCCESS);
+
+full:
+	if (nmax == DNS_NAME_MAXWIRE) {
+		/*
+		 * The name did not fit even though we had a buffer
+		 * big enough to fit a maximum-length name.
+		 */
+		return (DNS_R_NAMETOOLONG);
+	} else {
+		/*
+		 * The name might fit if only the caller could give us a
+		 * big enough buffer.
+		 */
+		return (ISC_R_NOSPACE);
+	}
+}

fuzz/old.h

@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+#pragma once
+
+/*%
+ * For verifying no functional change in the rewrite of dns_name_fromwire()
+ */
+isc_result_t
+old_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx,
+		  unsigned int options, isc_buffer_t *target);

lib/dns/include/dns/name.h

@@ -31,9 +31,9 @@
  * makes adding names to messages easy.  Having much of the server know
  * the representation would be perilous, and we certainly don't want each
  * user of names to be manipulating such a low-level structure.  This is
- * where the Names and Labels module comes in.  The module allows name or
- * label handles to be created and attached to uncompressed wire format
- * regions.  All name operations and conversions are done through these
+ * where the Names and Labels module comes in. The module allows name
+ * handles to be created and attached to uncompressed wire format
+ * regions. All name operations and conversions are done through these
  * handles.
  *
  * MP:
@@ -56,7 +56,6 @@
  * Standards:
  *\li	RFC1035
  *\li	Draft EDNS0 (0)
- *\li	Draft Binary Labels (2)
  *
  */
 
@@ -76,18 +75,11 @@
 
 ISC_LANG_BEGINDECLS
 
-/*****
-***** Labels
-*****
-***** A 'label' is basically a region.  It contains one DNS wire format
-***** label of type 00 (ordinary).
-*****/
-
 /*****
 ***** Names
 *****
 ***** A 'name' is a handle to a binary region.  It contains a sequence of one
-***** or more DNS wire format labels of type 00 (ordinary).
+***** or more DNS wire format labels.
 ***** Note that all names are not required to end with the root label,
 ***** as they are in the actual DNS wire protocol.
 *****/
@@ -697,9 +689,6 @@ dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx,
  * Notes:
  * \li	Decompression policy is controlled by 'dctx'.
  *
- * \li	If DNS_NAME_DOWNCASE is set, any uppercase letters in 'source' will be
- *	downcased when they are copied into 'target'.
- *
  * Security:
  *
  * \li	*** WARNING ***
@@ -720,13 +709,12 @@ dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx,
  *
  * \li	'dctx' is a valid decompression context.
  *
+ * \li	DNS_NAME_DOWNCASE is not set.
+ *
  * Ensures:
  *
  *	If result is success:
- * \li	 	If 'target' is not NULL, 'name' is attached to it.
- *
- * \li		Uppercase letters are downcased in the copy iff
- *		DNS_NAME_DOWNCASE is set in options.
+ * \li		If 'target' is not NULL, 'name' is attached to it.
  *
  * \li		The current location in source is advanced, and the used space
  *		in target is updated.
@@ -739,7 +727,6 @@ dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx,
  * \li	Bad Form: Compression type not allowed
  * \li	Bad Form: Bad compression pointer
  * \li	Bad Form: Input too short
- * \li	Resource Limit: Too many compression pointers
  * \li	Resource Limit: Not enough space in buffer
  */
 

lib/dns/name.c

@@ -47,8 +47,6 @@ typedef enum {
 	ft_at
 } ft_state;
 
-typedef enum { fw_start = 0, fw_ordinary, fw_newcurrent } fw_state;
-
 #define INIT_OFFSETS(name, var, default_offsets) \
 	if ((name)->offsets != NULL)             \
 		var = (name)->offsets;           \
@@ -1520,175 +1518,174 @@ set_offsets(const dns_name_t *name, unsigned char *offsets,
 }
 
 isc_result_t
-dns_name_fromwire(dns_name_t *name, isc_buffer_t *source, dns_decompress_t dctx,
-		  unsigned int options, isc_buffer_t *target) {
-	unsigned char *cdata, *ndata;
-	unsigned int cused; /* Bytes of compressed name data used */
-	unsigned int nused, labels, n, nmax;
-	unsigned int current, new_current, biggest_pointer;
-	bool done;
-	fw_state state = fw_start;
-	unsigned int c;
-	unsigned char *offsets;
-	dns_offsets_t odata;
-	bool downcase;
-	bool seen_pointer;
-
+dns_name_fromwire(dns_name_t *const name, isc_buffer_t *const source,
+		  const dns_decompress_t dctx, unsigned int options,
+		  isc_buffer_t *target) {
 	/*
-	 * Copy the possibly-compressed name at source into target,
-	 * decompressing it.  Loop prevention is performed by checking
-	 * the new pointer against biggest_pointer.
+	 * Copy the name at source into target, decompressing it.
+	 *
+	 *	*** WARNING ***
+	 *
+	 * dns_name_fromwire() deals with raw network data. An error in this
+	 * routine could result in the failure or hijacking of the server.
+	 *
+	 * The description of name compression in RFC 1035 section 4.1.4 is
+	 * subtle wrt certain edge cases. The first important sentence is:
+	 *
+	 * > In this scheme, an entire domain name or a list of labels at the
+	 * > end of a domain name is replaced with a pointer to a prior
+	 * > occurance of the same name.
+	 *
+	 * The key word is "prior". This says that compression pointers must
+	 * point strictly earlier in the message (before our "marker" variable),
+	 * which is enough to prevent DoS attacks due to compression loops.
+	 *
+	 * The next important sentence is:
+	 *
+	 * > If a domain name is contained in a part of the message subject to a
+	 * > length field (such as the RDATA section of an RR), and compression
+	 * > is used, the length of the compressed name is used in the length
+	 * > calculation, rather than the length of the expanded name.
+	 *
+	 * When decompressing, this means that the amount of the source buffer
+	 * that we consumed (which is checked wrt the container's length field)
+	 * is the length of the compressed name. A compressed name is defined as
+	 * a sequence of labels ending with the root label or a compression
+	 * pointer, that is, the segment of the name that dns_name_fromwire()
+	 * examines first.
+	 *
+	 * This matters when handling names that play dirty tricks, like:
+	 *
+	 *	+---+---+---+---+---+---+
+	 *	| 4 | 1 |'a'|192| 0 | 0 |
+	 *	+---+---+---+---+---+---+
+	 *
+	 * We start at octet 1. There is an ordinary single character label "a",
+	 * followed by a compression pointer that refers back to octet zero.
+	 * Here there is a label of length 4, which weirdly re-uses the octets
+	 * we already examined as the data for the label. It is followed by the
+	 * root label,
+	 *
+	 * The specification says that the compressed name ends after the first
+	 * zero octet (after the compression pointer) not the second zero octet,
+	 * even though the second octet is later in the message. This shows the
+	 * correct way to set our "consumed" variable.
 	 */
 
+	REQUIRE((options & DNS_NAME_DOWNCASE) == 0);
 	REQUIRE(VALID_NAME(name));
+	REQUIRE(BINDABLE(name));
 	REQUIRE((target != NULL && ISC_BUFFER_VALID(target)) ||
 		(target == NULL && ISC_BUFFER_VALID(name->buffer)));
 
-	downcase = ((options & DNS_NAME_DOWNCASE) != 0);
-
 	if (target == NULL && name->buffer != NULL) {
 		target = name->buffer;
 		isc_buffer_clear(target);
 	}
 
-	REQUIRE(BINDABLE(name));
+	uint8_t *const name_buf = isc_buffer_used(target);
+	const uint32_t name_max = ISC_MIN(DNS_NAME_MAXWIRE,
+					  isc_buffer_availablelength(target));
+	uint32_t name_len = 0;
+	MAKE_EMPTY(name); /* in case of failure */
 
+	dns_offsets_t odata;
+	uint8_t *offsets = NULL;
+	uint32_t labels = 0;
 	INIT_OFFSETS(name, offsets, odata);
 
 	/*
-	 * Make 'name' empty in case of failure.
+	 * After chasing a compression pointer, these variables refer to the
+	 * source buffer as follows:
+	 *
+	 * sb --- mr --- cr --- st --- cd --- sm
+	 *
+	 * sb = source_buf (const)
+	 * mr = marker
+	 * cr = cursor
+	 * st = start (const)
+	 * cd = consumed
+	 * sm = source_max (const)
+	 *
+	 * The marker hops backwards for each pointer.
+	 * The cursor steps forwards for each label.
+	 * The amount of the source we consumed is set once.
 	 */
-	MAKE_EMPTY(name);
+	const uint8_t *const source_buf = isc_buffer_base(source);
+	const uint8_t *const source_max = isc_buffer_used(source);
+	const uint8_t *const start = isc_buffer_current(source);
+	const uint8_t *marker = start;
+	const uint8_t *cursor = start;
+	const uint8_t *consumed = NULL;
 
 	/*
-	 * Initialize things to make the compiler happy; they're not required.
+	 * One iteration per label.
 	 */
-	n = 0;
-	new_current = 0;
-
-	/*
-	 * Set up.
-	 */
-	labels = 0;
-	done = false;
-
-	ndata = isc_buffer_used(target);
-	nused = 0;
-	seen_pointer = false;
-
-	/*
-	 * Find the maximum number of uncompressed target name
-	 * bytes we are willing to generate.  This is the smaller
-	 * of the available target buffer length and the
-	 * maximum legal domain name length (255).
-	 */
-	nmax = isc_buffer_availablelength(target);
-	if (nmax > DNS_NAME_MAXWIRE) {
-		nmax = DNS_NAME_MAXWIRE;
-	}
-
-	cdata = isc_buffer_current(source);
-	cused = 0;
-
-	current = source->current;
-	biggest_pointer = current;
-
-	/*
-	 * Note:  The following code is not optimized for speed, but
-	 * rather for correctness.  Speed will be addressed in the future.
-	 */
-
-	while (current < source->active && !done) {
-		c = *cdata++;
-		current++;
-		if (!seen_pointer) {
-			cused++;
-		}
-
-		switch (state) {
-		case fw_start:
-			if (c < 64) {
-				offsets[labels] = nused;
-				labels++;
-				if (nused + c + 1 > nmax) {
-					goto full;
-				}
-				nused += c + 1;
-				*ndata++ = c;
-				if (c == 0) {
-					done = true;
-				}
-				n = c;
-				state = fw_ordinary;
-			} else if (c >= 192) {
-				/*
-				 * 14-bit compression pointer
-				 */
-				if (!dns_decompress_getpermitted(dctx)) {
-					return (DNS_R_DISALLOWED);
-				}
-				new_current = c & 0x3F;
-				state = fw_newcurrent;
-			} else {
-				return (DNS_R_BADLABELTYPE);
+	while (cursor < source_max) {
+		const uint8_t label_len = *cursor++;
+		if (label_len < 64) {
+			/*
+			 * Normal label: record its offset, and check bounds on
+			 * the name length, which also ensures we don't overrun
+			 * the offsets array. Don't touch any source bytes yet!
+			 * The source bounds check will happen when we loop.
+			 */
+			offsets[labels++] = name_len;
+			/* and then a step to the ri-i-i-i-i-ight */
+			cursor += label_len;
+			name_len += label_len + 1;
+			if (name_len > name_max) {
+				return (name_max == DNS_NAME_MAXWIRE
+						? DNS_R_NAMETOOLONG
+						: ISC_R_NOSPACE);
+			} else if (label_len == 0) {
+				goto root_label;
 			}
-			break;
-		case fw_ordinary:
-			if (downcase) {
-				c = isc_ascii_tolower(c);
-			}
-			*ndata++ = c;
-			n--;
-			if (n == 0) {
-				state = fw_start;
-			}
-			break;
-		case fw_newcurrent:
-			new_current *= 256;
-			new_current += c;
-			if (new_current >= biggest_pointer) {
+		} else if (label_len < 192) {
+			return (DNS_R_BADLABELTYPE);
+		} else if (!dns_decompress_getpermitted(dctx)) {
+			return (DNS_R_DISALLOWED);
+		} else if (cursor < source_max) {
+			/*
+			 * Compression pointer. Ensure it does not loop.
+			 *
+			 * Copy multiple labels in one go, to make the most of
+			 * memmove() performance. Start at the marker and finish
+			 * just before the pointer's hi+lo bytes, before the
+			 * cursor. Bounds were already checked.
+			 */
+			const uint32_t hi = label_len & 0x3F;
+			const uint32_t lo = *cursor++;
+			const uint8_t *pointer = source_buf + (256 * hi + lo);
+			if (pointer >= marker) {
 				return (DNS_R_BADPOINTER);
 			}
-			biggest_pointer = new_current;
-			current = new_current;
-			cdata = (unsigned char *)source->base + current;
-			seen_pointer = true;
-			state = fw_start;
-			break;
-		default:
-			FATAL_ERROR("Unknown state %d", state);
-			/* Does not return. */
+			const uint32_t copy_len = (cursor - 2) - marker;
+			uint8_t *const dest = name_buf + name_len - copy_len;
+			memmove(dest, marker, copy_len);
+			consumed = consumed != NULL ? consumed : cursor;
+			/* it's just a jump to the left */
+			cursor = marker = pointer;
 		}
 	}
+	return (ISC_R_UNEXPECTEDEND);
+root_label:;
+	/*
+	 * Copy labels almost like we do for compression pointers,
+	 * from the marker up to and including the root label.
+	 */
+	const uint32_t copy_len = cursor - marker;
+	memmove(name_buf + name_len - copy_len, marker, copy_len);
+	consumed = consumed != NULL ? consumed : cursor;
+	isc_buffer_forward(source, consumed - start);
 
-	if (!done) {
-		return (ISC_R_UNEXPECTEDEND);
-	}
-
-	name->ndata = (unsigned char *)target->base + target->used;
-	name->labels = labels;
-	name->length = nused;
 	name->attributes.absolute = true;
-
-	isc_buffer_forward(source, cused);
-	isc_buffer_add(target, name->length);
+	name->ndata = name_buf;
+	name->labels = labels;
+	name->length = name_len;
+	isc_buffer_add(target, name_len);
 
 	return (ISC_R_SUCCESS);
-
-full:
-	if (nmax == DNS_NAME_MAXWIRE) {
-		/*
-		 * The name did not fit even though we had a buffer
-		 * big enough to fit a maximum-length name.
-		 */
-		return (DNS_R_NAMETOOLONG);
-	} else {
-		/*
-		 * The name might fit if only the caller could give us a
-		 * big enough buffer.
-		 */
-		return (ISC_R_NOSPACE);
-	}
 }
 
 isc_result_t

tests/bench/.gitignore

@@ -1,3 +1,4 @@
 /ascii
 /compress
+/dns_name_fromwire
 /siphash

tests/bench/Makefile.am

@@ -2,7 +2,8 @@ include $(top_srcdir)/Makefile.top
 
 AM_CPPFLAGS +=			\
 	$(LIBISC_CFLAGS)	\
-	$(LIBDNS_CFLAGS)
+	$(LIBDNS_CFLAGS)	\
+	-I$(top_srcdir)/fuzz
 
 LDADD +=			\
 	$(LIBISC_LIBS)		\
@@ -11,4 +12,10 @@ LDADD +=			\
 noinst_PROGRAMS =		\
 	ascii			\
 	compress		\
+	dns_name_fromwire	\
 	siphash
+
+dns_name_fromwire_SOURCES =		\
+	$(top_builddir)/fuzz/old.c	\
+	$(top_builddir)/fuzz/old.h	\
+	dns_name_fromwire.c

tests/bench/dns_name_fromwire.c

@@ -0,0 +1,145 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+#include <stddef.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <isc/ascii.h>
+#include <isc/buffer.h>
+#include <isc/random.h>
+#include <isc/time.h>
+#include <isc/util.h>
+
+#include <dns/compress.h>
+#include <dns/fixedname.h>
+#include <dns/name.h>
+
+#include "old.h"
+
+static uint32_t
+old_bench(const uint8_t *data, size_t size) {
+	isc_result_t result;
+	dns_fixedname_t fixed;
+	dns_name_t *name = dns_fixedname_initname(&fixed);
+	dns_decompress_t dctx = DNS_DECOMPRESS_PERMITTED;
+	isc_buffer_t buf;
+	uint32_t count = 0;
+
+	isc_buffer_constinit(&buf, data, size);
+	isc_buffer_add(&buf, size);
+	isc_buffer_setactive(&buf, size);
+
+	while (isc_buffer_consumedlength(&buf) < size) {
+		result = old_name_fromwire(name, &buf, dctx, 0, NULL);
+		if (result != ISC_R_SUCCESS) {
+			isc_buffer_forward(&buf, 1);
+		}
+		count++;
+	}
+	return (count);
+}
+
+static uint32_t
+new_bench(const uint8_t *data, size_t size) {
+	isc_result_t result;
+	dns_fixedname_t fixed;
+	dns_name_t *name = dns_fixedname_initname(&fixed);
+	dns_decompress_t dctx = DNS_DECOMPRESS_PERMITTED;
+	isc_buffer_t buf;
+	uint32_t count = 0;
+
+	isc_buffer_constinit(&buf, data, size);
+	isc_buffer_add(&buf, size);
+	isc_buffer_setactive(&buf, size);
+
+	while (isc_buffer_consumedlength(&buf) < size) {
+		result = dns_name_fromwire(name, &buf, dctx, 0, NULL);
+		if (result != ISC_R_SUCCESS) {
+			isc_buffer_forward(&buf, 1);
+		}
+		count++;
+	}
+	return (count);
+}
+
+static void
+oldnew_bench(const uint8_t *data, size_t size) {
+	isc_time_t t0;
+	isc_time_now_hires(&t0);
+	uint32_t n1 = old_bench(data, size);
+	isc_time_t t1;
+	isc_time_now_hires(&t1);
+	uint32_t n2 = new_bench(data, size);
+	isc_time_t t2;
+	isc_time_now_hires(&t2);
+
+	double t01 = (double)isc_time_microdiff(&t1, &t0);
+	double t12 = (double)isc_time_microdiff(&t2, &t1);
+	printf("  old %u / %f ms; %f / us\n", n1, t01 / 1000.0, n1 / t01);
+	printf("  new %u / %f ms; %f / us\n", n2, t12 / 1000.0, n2 / t12);
+	printf("  old/new %f or %f\n", t01 / t12, t12 / t01);
+}
+
+#define NAMES 1000
+static uint8_t buf[1024 * NAMES];
+
+int
+main(void) {
+	unsigned int p;
+
+	printf("random buffer\n");
+	isc_random_buf(buf, sizeof(buf));
+	oldnew_bench(buf, sizeof(buf));
+
+	p = 0;
+	for (unsigned int name = 0; name < NAMES; name++) {
+		unsigned int start = p;
+		unsigned int prev = p;
+		buf[p++] = 0;
+		for (unsigned int label = 0; label < 127; label++) {
+			unsigned int ptr = prev - start;
+			prev = p;
+			buf[p++] = 1;
+			buf[p++] = 'a';
+			buf[p++] = 0xC0 | (ptr >> 8);
+			buf[p++] = 0xFF & ptr;
+		}
+	}
+	printf("127 compression pointers\n");
+	oldnew_bench(buf, p);
+
+	p = 0;
+	for (unsigned int name = 0; name < NAMES; name++) {
+		for (unsigned int label = 0; label < 127; label++) {
+			buf[p++] = 1;
+			buf[p++] = 'a';
+		}
+		buf[p++] = 0;
+	}
+	printf("127 sequential labels\n");
+	oldnew_bench(buf, p);
+
+	p = 0;
+	for (unsigned int name = 0; name < NAMES; name++) {
+		for (unsigned int label = 0; label < 4; label++) {
+			buf[p++] = 62;
+			for (unsigned int c = 0; c < 62; c++) {
+				buf[p++] = 'a';
+			}
+		}
+		buf[p++] = 0;
+	}
+	printf("4 long sequential labels\n");
+	oldnew_bench(buf, p);
+}

tests/dns/name_test.c

@@ -500,11 +500,20 @@ ISC_RUN_TEST_IMPL(istat) {
 	}
 }
 
+static bool
+name_attr_zero(struct dns_name_attrs attributes) {
+	return (!(attributes.absolute | attributes.readonly |
+		  attributes.dynamic | attributes.dynoffsets |
+		  attributes.nocompress | attributes.cache | attributes.answer |
+		  attributes.ncache | attributes.chaining | attributes.chase |
+		  attributes.wildcard | attributes.prerequisite |
+		  attributes.update | attributes.hasupdaterec));
+}
+
 /* dns_nane_init */
 ISC_RUN_TEST_IMPL(init) {
 	dns_name_t name;
 	unsigned char offsets[1];
-	struct dns_name_attrs zeroes = {};
 
 	UNUSED(state);
 
@@ -513,16 +522,15 @@ ISC_RUN_TEST_IMPL(init) {
 	assert_null(name.ndata);
 	assert_int_equal(name.length, 0);
 	assert_int_equal(name.labels, 0);
-	assert_memory_equal(&name.attributes, &zeroes, sizeof(zeroes));
 	assert_ptr_equal(name.offsets, offsets);
 	assert_null(name.buffer);
+	assert_true(name_attr_zero(name.attributes));
 }
 
 /* dns_nane_invalidate */
 ISC_RUN_TEST_IMPL(invalidate) {
 	dns_name_t name;
 	unsigned char offsets[1];
-	struct dns_name_attrs zeroes = {};
 
 	UNUSED(state);
 
@@ -532,9 +540,9 @@ ISC_RUN_TEST_IMPL(invalidate) {
 	assert_null(name.ndata);
 	assert_int_equal(name.length, 0);
 	assert_int_equal(name.labels, 0);
-	assert_memory_equal(&name.attributes, &zeroes, sizeof(zeroes));
 	assert_null(name.offsets);
 	assert_null(name.buffer);
+	assert_true(name_attr_zero(name.attributes));
 }
 
 /* dns_nane_setbuffer/hasbuffer */