From a95358da6fe776ea305a97e6c5034c1f5b38b9bb Mon Sep 17 00:00:00 2001 From: Christian Glombek Date: Sat, 11 Jul 2026 17:31:15 +0200 Subject: [PATCH 1/3] dns-packet: keep unreferenceable names out of the compression map dns_packet_append_name() added every name it appends to the packet's compression map, including names written at offsets >= 0x4000 that an RFC 1035 compression pointer (14 bits) can never reference. Such entries are not just useless -- when the same name occurs again, the lookup finds it, the pointer cannot be emitted, the code falls through to appending the labels again, and the re-insertion into the map then fails the whole packet build with hashmap_ensure_put() returning -EEXIST. This is easily triggered with mDNS: an unsolicited DNS-SD announcement covering a zone with a few hundred services exceeds 16KiB, and dns_scope_announce() then fails with "Failed to build reply packet: File exists". This lifts the failure only: dns_scope_announce() still builds the whole zone into one packet and hands it to the socket as a single datagram, fragmented as the link requires. Splitting announcements across MTU-sized packets, as RFC 6762 section 17 asks, is done separately. Only insert a name into the map if its offset is pointer-expressible, making "mapped implies referenceable" a map invariant. Re-occurrences of out-of-range names are emitted as labels again -- the only encoding RFC 1035 section 4.1.4 permits in that case -- still compressed from the first suffix that is mapped in range. The lookup restates that range as control flow rather than asserting it. Under -Db_ndebug=true, which CI builds, assert() is __builtin_unreachable(), so an out-of-range entry would put 0xC000 | n truncated to 14 bits on the wire -- a pointer to an arbitrary earlier offset -- where falling through emits the labels instead. This layer is shared with unicast DNS, DoT and LLMNR. The pointer's tag bits and its offset bound are now named once, in dns-def.h next to the other wire-format constants rather than in dns-packet.h, since the format is RFC 1035's and not this packet abstraction's -- dhcp-message.c open-codes the same two bits to parse compressed names out of DHCP options. Compile-time checks pin that the two partition the 16-bit field and that the tag is its top two bits, which is what lets the reader take it from the first octet alone. The offset's other bound becomes control flow along with it: it was an assert(), which under -Db_ndebug=true is no check at all, for the bound whose violation puts a pointer to the wrong bytes on the wire. It is also tightened to the start of the name being appended rather than the end of the packet, which is the "prior occurrence" RFC 1035 section 4.1.4 allows a pointer to address and what dns_packet_read_name() enforces on the way in. Add regression tests, including one through dns_packet_append_rr() -- the path the reported failure came from, where the name append is nested inside the rdlength backpatch and two further rollbacks -- and one for the reader's label-type dispatch, which the renamed constants touch. --- src/basic/dns-def.h | 10 ++ src/resolve/test-dns-packet-append.c | 162 +++++++++++++++++++++++++++ src/shared/dns-packet.c | 35 +++--- 3 files changed, 192 insertions(+), 15 deletions(-) diff --git a/src/basic/dns-def.h b/src/basic/dns-def.h index 5c0f031df7..67276bb699 100644 --- a/src/basic/dns-def.h +++ b/src/basic/dns-def.h @@ -17,3 +17,13 @@ /* Maximum number of labels per valid hostname */ #define DNS_N_LABELS_MAX 127 + +/* RFC 1035 § 4.1.4 name compression: the two high bits tag a 16-bit field as a pointer, the + * remaining 14 bits carry the offset, which is hence also the last position a name a pointer can + * reference may start at. */ +#define DNS_COMPRESSION_POINTER_FLAG UINT16_C(0xC000) +#define DNS_COMPRESSION_OFFSET_MAX UINT16_C(0x3FFF) +assert_cc((DNS_COMPRESSION_POINTER_FLAG & DNS_COMPRESSION_OFFSET_MAX) == 0); +assert_cc((DNS_COMPRESSION_POINTER_FLAG | DNS_COMPRESSION_OFFSET_MAX) == UINT16_MAX); +/* The tag is the top two bits, which is what lets a reader take it from the first octet alone. */ +assert_cc((DNS_COMPRESSION_POINTER_FLAG & UINT16_C(0xFF)) == 0); diff --git a/src/resolve/test-dns-packet-append.c b/src/resolve/test-dns-packet-append.c index 862f3bea69..8615341b80 100644 --- a/src/resolve/test-dns-packet-append.c +++ b/src/resolve/test-dns-packet-append.c @@ -1,12 +1,16 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "alloc-util.h" #include "dns-answer.h" +#include "dns-def.h" #include "dns-packet.h" #include "dns-question.h" #include "dns-rr.h" #include "dns-type.h" +#include "hashmap.h" #include "list.h" #include "log.h" +#include "stdio-util.h" #include "tests.h" #define BIT_QR (1 << 7) @@ -1298,4 +1302,162 @@ TEST(packet_append_key_name_too_long) { ASSERT_EQ(packet->size, 12U); } +/* Pad the packet to exactly `target` bytes, so the next append starts at a chosen offset. Opaque + * filler: nothing parses it, and it leaves the compression map empty. */ +static void pad_packet_to(DnsPacket *p, size_t target) { + _cleanup_free_ void *filler = NULL; + size_t gap; + + ASSERT_GT(target, p->size); + gap = target - p->size; + ASSERT_NOT_NULL(filler = malloc0(gap)); + ASSERT_OK(dns_packet_append_blob(p, filler, gap, NULL)); + ASSERT_EQ(p->size, target); + ASSERT_EQ(hashmap_size(p->names), 0u); +} + +/* The reader's label-type dispatch. RFC 1035 section 4.1.4 gives a length octet the top bits 00 and + * a compression pointer 11, and reserves 01 and 10: it takes both bits to make a pointer. The + * reserved octet points at a real name here, so a dispatch accepting either bit follows it and + * parses that name; only rejecting the octet outright gives -EBADMSG. */ +TEST(packet_read_name_rejects_reserved_label_types) { + uint8_t first; + + FOREACH_ARGUMENT(first, 0x40, 0x80) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + _cleanup_free_ char *parsed = NULL; + const uint8_t reserved[] = { first, DNS_PACKET_HEADER_SIZE }; + size_t at; + + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, + DNS_PACKET_SIZE_MAX)); + ASSERT_OK(dns_packet_append_name(packet, "example.org", + /* allow_compression= */ false, + /* canonical_candidate= */ false, + NULL)); + + at = packet->size; + ASSERT_OK(dns_packet_append_blob(packet, reserved, sizeof(reserved), NULL)); + dns_packet_rewind(packet, at); + + ASSERT_ERROR(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, + NULL), EBADMSG); + } +} + +TEST(packet_append_name_beyond_compression_pointer_range) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + size_t start; + + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + + /* Fill the packet past the range a compression pointer can express (RFC 1035 pointers carry + * 14 bits, i.e. offsets up to 0x3fff) with unique names. */ + for (size_t i = 0; packet->size <= DNS_COMPRESSION_OFFSET_MAX; i++) { + char name[64]; + + xsprintf(name, "filler%zu.example.com", i); + ASSERT_OK(dns_packet_append_name(packet, name, + /* allow_compression= */ true, + /* canonical_candidate= */ false, + NULL)); + } + + /* The first occurrence of this name lands beyond pointer range, so it must not be added to + * the compression map... */ + size_t n_mapped = hashmap_size(packet->names); + ASSERT_OK(dns_packet_append_name(packet, "far.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + &start)); + ASSERT_GT(start, (size_t) DNS_COMPRESSION_OFFSET_MAX); + ASSERT_EQ(hashmap_size(packet->names), n_mapped); + + /* ...and since no pointer can reference it, later occurrences must be appended as labels + * again. This used to fail with -EEXIST when the name was reinserted into the map. */ + size_t second; + ASSERT_OK(dns_packet_append_name(packet, "far.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + &second)); + ASSERT_OK(dns_packet_append_name(packet, "far.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL)); + ASSERT_EQ(hashmap_size(packet->names), n_mapped); + + /* Names first seen within pointer range still compress: re-appending one takes exactly the + * two bytes of a compression pointer. */ + size_t before = packet->size; + ASSERT_OK(dns_packet_append_name(packet, "filler0.example.com", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL)); + ASSERT_EQ(packet->size, before + 2); + + /* Both encodings must round-trip through the parser: the label-form re-occurrence beyond + * pointer range and the pointer-form one within it. */ + _cleanup_free_ char *parsed = NULL; + dns_packet_rewind(packet, second); + ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL)); + ASSERT_STREQ(parsed, "far.example.org"); + + parsed = mfree(parsed); + dns_packet_rewind(packet, before); + ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL)); + ASSERT_STREQ(parsed, "filler0.example.com"); +} + +/* The same case via dns_packet_append_rr(), where the name append is nested inside the rdlength + * backpatch -- the path the reported dns_scope_announce() failure came from. */ +TEST(packet_append_rr_beyond_compression_pointer_range) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + const size_t n_records = 8; + size_t first_rr = 0; + + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, DNS_PACKET_SIZE_MAX)); + + /* Start past the range a pointer can express, so every name these records carry -- owner + * and rdata alike -- has its first occurrence out of reach. */ + pad_packet_to(packet, DNS_COMPRESSION_OFFSET_MAX + 1); + + /* Distinct owners, one shared PTR target: the target recurs in every record, out of pointer + * range, so each occurrence is re-emitted as labels and none is re-inserted into the map. */ + for (size_t i = 0; i < n_records; i++) { + _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *ptr = NULL; + char name[64]; + size_t start; + + xsprintf(name, "%zu.instance.example.com", i); + ptr = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_PTR, name); + ASSERT_NOT_NULL(ptr); + ptr->ttl = 120; + ASSERT_NOT_NULL(ptr->ptr.name = strdup("shared.target.example.org")); + + ASSERT_OK(dns_packet_append_rr(packet, ptr, /* flags= */ 0, + &start, /* rdata_start= */ NULL)); + if (i == 0) + first_rr = start; + } + + /* Nothing was mapped: every name in this packet starts beyond the range a pointer can + * express, which is what makes the appends above the re-occurrence case. */ + ASSERT_EQ(hashmap_size(packet->names), 0u); + + /* And every record reads back with its rdata intact: the rdlength backpatch has to agree + * with a target written as labels rather than pointed at. (The packet is read from the + * first record rather than through dns_packet_extract(), since the padding in front of it + * is not a question section.) */ + dns_packet_rewind(packet, first_rr); + for (size_t i = 0; i < n_records; i++) { + _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *parsed = NULL; + char name[64]; + + ASSERT_OK(dns_packet_read_rr(packet, &parsed, /* ret_cache_flush= */ NULL, + /* start= */ NULL)); + ASSERT_NOT_NULL(parsed); + + xsprintf(name, "%zu.instance.example.com", i); + ASSERT_EQ(parsed->key->type, (uint16_t) DNS_TYPE_PTR); + ASSERT_STREQ(dns_resource_key_name(parsed->key), name); + ASSERT_STREQ(parsed->ptr.name, "shared.target.example.org"); + } +} + DEFINE_TEST_MAIN(LOG_DEBUG) diff --git a/src/shared/dns-packet.c b/src/shared/dns-packet.c index 429e5c82c1..43c50bfbad 100644 --- a/src/shared/dns-packet.c +++ b/src/shared/dns-packet.c @@ -632,31 +632,36 @@ int dns_packet_append_name( while (!dns_name_is_root(name)) { const char *z = name; char label[DNS_LABEL_MAX+1]; - size_t n = 0; + size_t n = 0, offset; if (allow_compression) n = PTR_TO_SIZE(hashmap_get(p->names, name)); - if (n > 0) { - assert(n < p->size); + /* Only pointer-expressible offsets of already-appended labels enter the map below, + * so both bounds should hold. Control flow and not assert(), which is no check at + * all under -Db_ndebug=true, for bounds that decide what goes on the wire. + * saved_size, not p->size: RFC 1035 § 4.1.4 allows a pointer only to a prior + * occurrence, which is what dns_packet_read_name() enforces on the way in. */ + if (n > 0 && n <= DNS_COMPRESSION_OFFSET_MAX && n < saved_size) { + r = dns_packet_append_uint16(p, DNS_COMPRESSION_POINTER_FLAG | n, NULL); + if (r < 0) + goto fail; - if (n < 0x4000) { - r = dns_packet_append_uint16(p, 0xC000 | n, NULL); - if (r < 0) - goto fail; - - goto done; - } + goto done; } r = dns_label_unescape(&name, label, sizeof label, 0); if (r < 0) goto fail; - r = dns_packet_append_label(p, label, r, canonical_candidate, &n); + r = dns_packet_append_label(p, label, r, canonical_candidate, &offset); if (r < 0) goto fail; - if (allow_compression) { + /* Remember the name for compression -- but only if this occurrence sits within the + * 14 bits an RFC 1035 pointer can express. An offset beyond that can never be + * referenced, so it doesn't belong in the map: it would only collide with a later + * occurrence of the same name, needlessly failing the whole append with -EEXIST. */ + if (allow_compression && offset <= DNS_COMPRESSION_OFFSET_MAX) { _cleanup_free_ char *s = NULL; if (!GREEDY_REALLOC(added_entries, n_added_entries + 1)) { @@ -670,7 +675,7 @@ int dns_packet_append_name( goto fail; } - r = hashmap_ensure_put(&p->names, &dns_name_hash_ops, s, SIZE_TO_PTR(n)); + r = hashmap_ensure_put(&p->names, &dns_name_hash_ops, s, SIZE_TO_PTR(offset)); if (r < 0) goto fail; @@ -1626,7 +1631,7 @@ int dns_packet_read_name( return -EBADMSG; continue; - } else if (allow_compression && FLAGS_SET(c, 0xc0)) { + } else if (allow_compression && FLAGS_SET(c, DNS_COMPRESSION_POINTER_FLAG >> 8)) { uint16_t ptr; /* Pointer */ @@ -1634,7 +1639,7 @@ int dns_packet_read_name( if (r < 0) return r; - ptr = (uint16_t) (c & ~0xc0) << 8 | (uint16_t) d; + ptr = (uint16_t) (c & ~(DNS_COMPRESSION_POINTER_FLAG >> 8)) << 8 | (uint16_t) d; if (ptr < DNS_PACKET_HEADER_SIZE || ptr >= jump_barrier) return -EBADMSG; From a4b109f01fd8060b60c1de73c0a420e0dd1dbb2b Mon Sep 17 00:00:00 2001 From: Christian Glombek Date: Tue, 25 Aug 2026 11:04:37 +0200 Subject: [PATCH 2/3] test: dns-packet: pin suffix compression past the pointer range The existing case only covered a fully-uncompressed re-occurrence. Every DNS-SD name shares an in-range suffix, so also pin that a name first seen beyond the pointer range still compresses from its mapped suffix -- one label plus a two-byte pointer per occurrence, map unchanged, and the pointer addressing the suffix's prior occurrence rather than any in-range octet. A padding helper puts the next append at a chosen offset, for the two sides of the boundary: at 0x3FFF the name is still mapped and re-occurs as a pointer whose wire bytes are checked independently of the parser; one past it, it is not mapped. --- src/resolve/test-dns-packet-append.c | 133 ++++++++++++++++++++++++--- 1 file changed, 119 insertions(+), 14 deletions(-) diff --git a/src/resolve/test-dns-packet-append.c b/src/resolve/test-dns-packet-append.c index 8615341b80..4c150a893d 100644 --- a/src/resolve/test-dns-packet-append.c +++ b/src/resolve/test-dns-packet-append.c @@ -12,6 +12,7 @@ #include "log.h" #include "stdio-util.h" #include "tests.h" +#include "unaligned.h" #define BIT_QR (1 << 7) #define BIT_AA (1 << 2) @@ -1302,20 +1303,6 @@ TEST(packet_append_key_name_too_long) { ASSERT_EQ(packet->size, 12U); } -/* Pad the packet to exactly `target` bytes, so the next append starts at a chosen offset. Opaque - * filler: nothing parses it, and it leaves the compression map empty. */ -static void pad_packet_to(DnsPacket *p, size_t target) { - _cleanup_free_ void *filler = NULL; - size_t gap; - - ASSERT_GT(target, p->size); - gap = target - p->size; - ASSERT_NOT_NULL(filler = malloc0(gap)); - ASSERT_OK(dns_packet_append_blob(p, filler, gap, NULL)); - ASSERT_EQ(p->size, target); - ASSERT_EQ(hashmap_size(p->names), 0u); -} - /* The reader's label-type dispatch. RFC 1035 section 4.1.4 gives a length octet the top bits 00 and * a compression pointer 11, and reserves 01 and 10: it takes both bits to make a pointer. The * reserved octet points at a real name here, so a dispatch accepting either bit follows it and @@ -1345,6 +1332,14 @@ TEST(packet_read_name_rejects_reserved_label_types) { } } +/* Every offset in the compression map must be pointer-expressible. */ +static void assert_offsets_in_pointer_range(const DnsPacket *p) { + void *v; + + HASHMAP_FOREACH(v, p->names) + ASSERT_LE(PTR_TO_SIZE(v), (size_t) DNS_COMPRESSION_OFFSET_MAX); +} + TEST(packet_append_name_beyond_compression_pointer_range) { _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; size_t start; @@ -1382,6 +1377,7 @@ TEST(packet_append_name_beyond_compression_pointer_range) { /* allow_compression= */ true, /* canonical_candidate= */ false, NULL)); ASSERT_EQ(hashmap_size(packet->names), n_mapped); + assert_offsets_in_pointer_range(packet); /* Names first seen within pointer range still compress: re-appending one takes exactly the * two bytes of a compression pointer. */ @@ -1402,6 +1398,115 @@ TEST(packet_append_name_beyond_compression_pointer_range) { dns_packet_rewind(packet, before); ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL)); ASSERT_STREQ(parsed, "filler0.example.com"); + + /* A name beyond the pointer range whose suffix IS mapped in range -- the shape every DNS-SD + * name hits via ".local" -- must still compress from that suffix: one label plus a two-byte + * pointer, on every occurrence, with the map unchanged. */ + n_mapped = hashmap_size(packet->names); + for (size_t i = 0; i < 2; i++) { + size_t at, occurrence_start = packet->size; + + ASSERT_OK(dns_packet_append_name(packet, "far.example.com", + /* allow_compression= */ true, + /* canonical_candidate= */ false, + &at)); + ASSERT_GT(at, (size_t) DNS_COMPRESSION_OFFSET_MAX); + ASSERT_EQ(packet->size, occurrence_start + 1 + STRLEN("far") + 2); + ASSERT_EQ(hashmap_size(packet->names), n_mapped); + assert_offsets_in_pointer_range(packet); + + /* The pointer must address the suffix's mapped prior occurrence, not just any + * in-range length octet (RFC 1035 section 4.1.4): compare it with the map. */ + size_t suffix = PTR_TO_SIZE(hashmap_get(packet->names, "example.com")); + size_t pointer_at = occurrence_start + 1 + STRLEN("far"); + ASSERT_GT(suffix, 0u); + ASSERT_LT(suffix, occurrence_start); + ASSERT_EQ(unaligned_read_be16(DNS_PACKET_DATA(packet) + pointer_at), + (uint16_t) (DNS_COMPRESSION_POINTER_FLAG | suffix)); + + parsed = mfree(parsed); + dns_packet_rewind(packet, at); + ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL)); + ASSERT_STREQ(parsed, "far.example.com"); + } +} + +/* Pad the packet to exactly `target` bytes, so the next append starts at a chosen offset. Opaque + * filler: nothing parses it, and it leaves the compression map empty. */ +static void pad_packet_to(DnsPacket *p, size_t target) { + _cleanup_free_ void *filler = NULL; + size_t gap; + + ASSERT_GT(target, p->size); + gap = target - p->size; + ASSERT_NOT_NULL(filler = malloc0(gap)); + ASSERT_OK(dns_packet_append_blob(p, filler, gap, NULL)); + ASSERT_EQ(p->size, target); + ASSERT_EQ(hashmap_size(p->names), 0u); +} + +TEST(packet_append_name_at_compression_offset_max) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + size_t start; + + /* A name whose first label starts at exactly the last pointer-expressible offset must be + * mapped and referenced by pointer... */ + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + pad_packet_to(packet, DNS_COMPRESSION_OFFSET_MAX); + + ASSERT_OK(dns_packet_append_name(packet, "edge.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + &start)); + ASSERT_EQ(start, (size_t) DNS_COMPRESSION_OFFSET_MAX); + /* Only the whole name is mapped: its suffixes start past the bound. */ + ASSERT_EQ(hashmap_size(packet->names), 1u); + + size_t before = packet->size; + ASSERT_OK(dns_packet_append_name(packet, "edge.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL)); + ASSERT_EQ(packet->size, before + 2); + /* The pointer's wire bytes, independent of the parser: offset 0x3FFF under the tag bits. */ + ASSERT_EQ(DNS_PACKET_DATA(packet)[before], 0xFFu); + ASSERT_EQ(DNS_PACKET_DATA(packet)[before + 1], 0xFFu); + + _cleanup_free_ char *parsed = NULL; + dns_packet_rewind(packet, before); + ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL)); + ASSERT_STREQ(parsed, "edge.example.org"); + + /* The same two bytes must be refused, not followed, where compression is not allowed. */ + parsed = mfree(parsed); + dns_packet_rewind(packet, before); + ASSERT_ERROR(dns_packet_read_name(packet, &parsed, /* allow_compression= */ false, NULL), + EBADMSG); +} + +TEST(packet_append_name_past_compression_offset_max) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + _cleanup_free_ char *parsed = NULL; + size_t start, before; + + /* One byte past the last pointer-expressible offset, a name stays out of the map and every + * occurrence is re-emitted as labels. */ + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX)); + pad_packet_to(packet, DNS_COMPRESSION_OFFSET_MAX + 1); + + ASSERT_OK(dns_packet_append_name(packet, "past.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + &start)); + ASSERT_EQ(start, (size_t) DNS_COMPRESSION_OFFSET_MAX + 1); + ASSERT_EQ(hashmap_size(packet->names), 0u); + + before = packet->size; + ASSERT_OK(dns_packet_append_name(packet, "past.example.org", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL)); + ASSERT_EQ(packet->size, before + STRLEN("past.example.org") + 2); + + dns_packet_rewind(packet, before); + ASSERT_OK(dns_packet_read_name(packet, &parsed, /* allow_compression= */ true, NULL)); + ASSERT_STREQ(parsed, "past.example.org"); } /* The same case via dns_packet_append_rr(), where the name append is nested inside the rdlength From 21de1795197c88c3df8f336f88478f454aa9e289 Mon Sep 17 00:00:00 2001 From: Christian Glombek Date: Mon, 7 Sep 2026 00:40:58 +0200 Subject: [PATCH 3/3] dns-packet: roll back the compression map when the root label fails dns_packet_append_name() appends a name's labels and then the root byte that terminates it. A failure on that last byte returned directly, leaving both the labels on the packet and the compression entries this call had added -- entries naming offsets the caller then truncates away. Take the same rollback path as every other failure in the function. Independent of the map invariant fixed before it: this is reachable whether or not any offset exceeded the pointer range. --- src/resolve/test-dns-packet-append.c | 63 ++++++++++++++++++++++++++++ src/shared/dns-packet.c | 2 +- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/resolve/test-dns-packet-append.c b/src/resolve/test-dns-packet-append.c index 4c150a893d..5bcace319b 100644 --- a/src/resolve/test-dns-packet-append.c +++ b/src/resolve/test-dns-packet-append.c @@ -1565,4 +1565,67 @@ TEST(packet_append_rr_beyond_compression_pointer_range) { } } +/* A packet with room for the labels but not the root byte, with one name and with two. */ +TEST(packet_append_name_rolls_back_on_failure) { + _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL; + size_t before; + + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, + /* max_size= */ DNS_PACKET_HEADER_SIZE + 4)); + before = packet->size; + + ASSERT_ERROR(dns_packet_append_name(packet, "abc", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL), EMSGSIZE); + + /* Nothing of the half-written name is left on the wire... */ + ASSERT_EQ(packet->size, before); + + /* ...and, the point of the rollback, no compression entry either: the label was mapped + * before the root byte failed, and an entry surviving here would name an offset that no + * longer holds what it claims. */ + ASSERT_EQ(hashmap_size(packet->names), 0u); + + /* Positive control: one more byte of budget and the same name fits, so the shortfall above + * is the root byte and not a label -- the root byte being the append under test here. */ + packet = dns_packet_unref(packet); + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, + /* max_size= */ DNS_PACKET_HEADER_SIZE + 5)); + ASSERT_OK(dns_packet_append_name(packet, "abc", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL)); + + /* Two labels, so more than one entry has to come back out: "a" and "b" fill the packet + * exactly and the root byte fails, with the map holding "a.b" and "b". */ + packet = dns_packet_unref(packet); + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, + /* max_size= */ DNS_PACKET_HEADER_SIZE + 4)); + before = packet->size; + + ASSERT_ERROR(dns_packet_append_name(packet, "a.b", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL), EMSGSIZE); + ASSERT_EQ(packet->size, before); + ASSERT_EQ(hashmap_size(packet->names), 0u); + + /* Only this call's entries come out. The map owns the keys, so removing one an earlier call + * inserted would free a key the map still holds -- and a wholesale clear would satisfy the + * assertions above, which never have anything else in the map. */ + packet = dns_packet_unref(packet); + ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, /* min_alloc_dsize= */ 0, + /* max_size= */ DNS_PACKET_HEADER_SIZE + 7)); + ASSERT_OK(dns_packet_append_name(packet, "ab", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL)); + ASSERT_EQ(hashmap_size(packet->names), 1u); + before = packet->size; + + ASSERT_ERROR(dns_packet_append_name(packet, "cd", + /* allow_compression= */ true, /* canonical_candidate= */ false, + NULL), EMSGSIZE); + ASSERT_EQ(packet->size, before); + ASSERT_EQ(hashmap_size(packet->names), 1u); + ASSERT_EQ(PTR_TO_SIZE(hashmap_get(packet->names, "ab")), (size_t) DNS_PACKET_HEADER_SIZE); +} + DEFINE_TEST_MAIN(LOG_DEBUG) diff --git a/src/shared/dns-packet.c b/src/shared/dns-packet.c index 43c50bfbad..bb3b8f9235 100644 --- a/src/shared/dns-packet.c +++ b/src/shared/dns-packet.c @@ -686,7 +686,7 @@ int dns_packet_append_name( r = dns_packet_append_uint8(p, 0, NULL); if (r < 0) - return r; + goto fail; done: if (start)