Commit 0ecacc6e83 for bind
commit 0ecacc6e83bf45c48ea96cbb67622ad15b83b48c
Author: Ondřej Surý <ondrej@isc.org>
Date: Sun May 3 08:02:00 2026 +0200
Fix dns_compress_rollback() count underflow and stale-entry skip
When dns_message_renderend() re-renders a truncated response with
TC + OPT/TSIG/SIG(0), it calls dns_compress_rollback(cctx, 0) to
discard every compression entry. Two defects in that walk: (1) the
unsigned comparison `set[slot].coff < 0` is always false, so empty
slots also entered the deletion path and decremented count once per
slot — underflowing the uint16_t and locking out later
insert_label() calls in the same render via the load-factor guard;
(2) after a deletion shifted entries down to preserve the probe
sequence, the outer scan's unconditional `slot++` skipped
re-examining the entry the cascade had just shifted into `slot`.
For coff = 0, memset the slot table — every entry is discarded
anyway. For coff > 0, walk the table once and only advance `slot`
when the inner cascade did not shift, so the entry now sitting in
`slot` is re-checked.
diff --git a/lib/dns/compress.c b/lib/dns/compress.c
index 9fc067a469..4f5a093dee 100644
--- a/lib/dns/compress.c
+++ b/lib/dns/compress.c
@@ -361,17 +361,31 @@ void
dns_compress_rollback(dns_compress_t *cctx, unsigned int coff) {
REQUIRE(CCTX_VALID(cctx));
- for (unsigned int slot = 0; slot <= cctx->mask; slot++) {
+ /*
+ * coff == 0 is the dns_message_renderend() TC + OPT/TSIG/SIG(0)
+ * re-render path: every entry is being discarded, so wipe the
+ * whole table in one shot rather than walking it slot by slot.
+ */
+ if (coff == 0) {
+ memset(cctx->set, 0,
+ (size_t)(cctx->mask + 1) * sizeof(*cctx->set));
+ cctx->count = 0;
+ return;
+ }
+
+ /*
+ * Selective rollback: remove every entry whose buffer offset is
+ * at or past `coff`. When entries are slid down to preserve the
+ * probe sequence, the entry shifted into `slot` must be
+ * re-examined — it may itself be eligible for deletion. Don't
+ * advance `slot` after a shift.
+ */
+ unsigned int slot = 0;
+ while (slot <= cctx->mask) {
if (cctx->set[slot].coff < coff) {
+ slot++;
continue;
}
- /*
- * The next few elements might be part of the deleted element's
- * probe sequence, so we slide them down to overwrite the entry
- * we are deleting and preserve the probe sequence. Moving an
- * element to the previous slot reduces its probe distance, so
- * we stop when we find an element whose probe distance is zero.
- */
unsigned int prev = slot;
unsigned int next = slot_index(cctx, prev, 1);
while (cctx->set[next].coff != 0 &&
@@ -384,5 +398,9 @@ dns_compress_rollback(dns_compress_t *cctx, unsigned int coff) {
cctx->set[prev].coff = 0;
cctx->set[prev].hash = 0;
cctx->count--;
+ if (prev == slot) {
+ slot++;
+ }
+ /* else: leave `slot` to re-examine the shifted entry */
}
}
diff --git a/tests/dns/compress_test.c b/tests/dns/compress_test.c
new file mode 100644
index 0000000000..e03f562fe3
--- /dev/null
+++ b/tests/dns/compress_test.c
@@ -0,0 +1,135 @@
+/*
+ * 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 <sched.h> /* IWYU pragma: keep */
+#include <setjmp.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#define UNIT_TESTING
+#include <cmocka.h>
+
+#include <isc/buffer.h>
+#include <isc/lib.h>
+#include <isc/mem.h>
+
+#include <dns/compress.h>
+#include <dns/fixedname.h>
+#include <dns/message.h>
+#include <dns/name.h>
+
+#include <tests/isc.h>
+
+/*
+ * dns_message_renderend() invokes dns_compress_rollback(cctx, 0) when a
+ * truncated response is re-rendered with TC+OPT/TSIG/SIG(0). The walk
+ * must clear every populated slot and leave count == 0 — and crucially
+ * must not decrement count for empty slots, which would underflow the
+ * uint16_t and lock out subsequent insert_label() calls in the same
+ * render via the load-factor guard.
+ */
+ISC_RUN_TEST_IMPL(rollback_zero_clears_all) {
+ dns_compress_t cctx;
+ isc_buffer_t buffer;
+ unsigned char bufdata[1024];
+ const char *names[] = {
+ "www.example.com.", "mail.example.com.", "ns1.example.com.",
+ "ns2.example.com.", "a.b.c.d.example.", "x.y.z.example.",
+ "foo.bar.baz.test.", "alpha.beta.gamma.",
+ };
+
+ isc_buffer_init(&buffer, bufdata, sizeof(bufdata));
+ isc_buffer_add(&buffer, DNS_MESSAGE_HEADERLEN);
+
+ dns_compress_init(&cctx, isc_g_mctx, 0);
+
+ for (size_t i = 0; i < ARRAY_SIZE(names); i++) {
+ dns_fixedname_t fixed;
+ dns_name_t *name = dns_fixedname_initname(&fixed);
+ assert_int_equal(
+ dns_name_fromstring(name, names[i], NULL, 0, NULL),
+ ISC_R_SUCCESS);
+
+ assert_int_equal(dns_name_towire(name, &cctx, &buffer),
+ ISC_R_SUCCESS);
+ }
+
+ assert_true(cctx.count > 0);
+
+ dns_compress_rollback(&cctx, 0);
+
+ assert_int_equal(cctx.count, 0);
+ for (unsigned int i = 0; i <= cctx.mask; i++) {
+ assert_int_equal(cctx.set[i].coff, 0);
+ assert_int_equal(cctx.set[i].hash, 0);
+ }
+
+ dns_compress_invalidate(&cctx);
+}
+
+/*
+ * Edge case: rollback(0) on a fresh, empty compression context must be
+ * a no-op. The pre-fix code decremented count for every empty slot,
+ * underflowing uint16_t to ~65535.
+ */
+ISC_RUN_TEST_IMPL(rollback_zero_on_empty) {
+ dns_compress_t cctx;
+
+ dns_compress_init(&cctx, isc_g_mctx, 0);
+ assert_int_equal(cctx.count, 0);
+
+ dns_compress_rollback(&cctx, 0);
+
+ assert_int_equal(cctx.count, 0);
+
+ dns_compress_invalidate(&cctx);
+}
+
+/*
+ * After rollback(0) the table must accept new insertions. Pre-fix, the
+ * underflowed count tripped the load-factor guard in insert_label() and
+ * silently dropped every subsequent compression entry.
+ */
+ISC_RUN_TEST_IMPL(rollback_zero_then_reuse) {
+ dns_compress_t cctx;
+ isc_buffer_t buffer;
+ unsigned char bufdata[1024];
+ dns_fixedname_t fixed;
+ dns_name_t *name = NULL;
+
+ isc_buffer_init(&buffer, bufdata, sizeof(bufdata));
+ isc_buffer_add(&buffer, DNS_MESSAGE_HEADERLEN);
+
+ dns_compress_init(&cctx, isc_g_mctx, 0);
+
+ dns_compress_rollback(&cctx, 0);
+
+ name = dns_fixedname_initname(&fixed);
+ assert_int_equal(dns_name_fromstring(name, "after.rollback.test.", NULL,
+ 0, NULL),
+ ISC_R_SUCCESS);
+ assert_int_equal(dns_name_towire(name, &cctx, &buffer), ISC_R_SUCCESS);
+ assert_true(cctx.count > 0);
+ assert_true(cctx.count <= cctx.mask + 1U);
+
+ dns_compress_invalidate(&cctx);
+}
+
+ISC_TEST_LIST_START
+ISC_TEST_ENTRY(rollback_zero_clears_all)
+ISC_TEST_ENTRY(rollback_zero_on_empty)
+ISC_TEST_ENTRY(rollback_zero_then_reuse)
+ISC_TEST_LIST_END
+ISC_TEST_MAIN
diff --git a/tests/dns/meson.build b/tests/dns/meson.build
index 54820af731..5d407d7497 100644
--- a/tests/dns/meson.build
+++ b/tests/dns/meson.build
@@ -13,6 +13,7 @@ dns_tests = [
'acl',
'badcache',
'byaddr',
+ 'compress',
'db',
'dbdiff',
'dbiterator',