From: Marek Vavruša <mvavrusa@cloudflare.com>
Date: Wed, 28 Mar 2018 05:10:09 +0000 (-0700)
Subject: cache: fixed crash with RR sets with over 255 records
X-Git-Tag: v2.2.0~2^2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a0c2e933f8d80fa9e3cf605a273b975aed7c4ba0;p=thirdparty%2Fknot-resolver.git

cache: fixed crash with RR sets with over 255 records

The previous cache version encoded RR count as uint8_t, which doesn't
work with RR sets with over 255 records. This caused cache writes
to fail and subsequently ending in an assertion failure.
It is not very common to have large RR sets, but it has legitimate
use cases such as a lot of SRV or address records for large container
deployments etc.
---

diff --git a/lib/cache/api.c b/lib/cache/api.c
index 01cec8e8f..8f584523c 100644
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -51,7 +51,7 @@
 
 
 /** Cache version */
-static const uint16_t CACHE_VERSION = 2;
+static const uint16_t CACHE_VERSION = 3;
 /** Key size */
 #define KEY_HSIZE (sizeof(uint8_t) + sizeof(uint16_t))
 #define KEY_SIZE (KEY_HSIZE + KNOT_DNAME_MAXLEN)
diff --git a/lib/cache/entry_list.c b/lib/cache/entry_list.c
index 9afdafee7..860ee1b3b 100644
--- a/lib/cache/entry_list.c
+++ b/lib/cache/entry_list.c
@@ -36,9 +36,10 @@ static int entry_h_len(const knot_db_val_t val)
 	if (!eh->is_packet) { /* Positive RRset + its RRsig set (may be empty). */
 		int sets = 2;
 		while (sets-- > 0) {
-			if (d + 1 > data_bound) return kr_error(EILSEQ);
-			uint8_t rr_count;
-			memcpy(&rr_count, d++, sizeof(rr_count));
+			if (d + 2 > data_bound) return kr_error(EILSEQ);
+			uint16_t rr_count;
+			memcpy(&rr_count, d, sizeof(rr_count));
+			d += sizeof(rr_count);
 			for (int i = 0; i < rr_count; ++i) {
 				if (d + 2 > data_bound) return kr_error(EILSEQ);
 				uint16_t len;
diff --git a/lib/cache/entry_rr.c b/lib/cache/entry_rr.c
index b0019def4..3d518009a 100644
--- a/lib/cache/entry_rr.c
+++ b/lib/cache/entry_rr.c
@@ -25,11 +25,12 @@ int rdataset_dematerialize(const knot_rdataset_t *rds, void * restrict data)
 {
 	//const void *data0 = data;
 	assert(data);
-	if (rds && rds->rr_count > 255) {
-		return kr_error(ENOSPC);
+	if (!data) {
+		return kr_error(EINVAL);
 	}
-	uint8_t rr_count = rds ? rds->rr_count : 0;
-	memcpy(data++, &rr_count, sizeof(rr_count));
+	uint16_t rr_count = rds ? rds->rr_count : 0;
+	memcpy(data, &rr_count, sizeof(rr_count));
+	data += sizeof(rr_count);
 
 	knot_rdata_t *rd = rds ? rds->data : NULL;
 	for (int i = 0; i < rr_count; ++i, rd = kr_rdataset_next(rd)) {
@@ -53,8 +54,9 @@ static int rdataset_materialize(knot_rdataset_t * restrict rds, const void * con
 	assert(pool); /* not required, but that's our current usage; guard leaks */
 	const void *d = data; /* iterates over the cache data */
 	{
-		uint8_t rr_count;
-		memcpy(&rr_count, d++, sizeof(rr_count));
+		uint16_t rr_count;
+		memcpy(&rr_count, d, sizeof(rr_count));
+		d += sizeof(rr_count);
 		rds->rr_count = rr_count;
 		if (!rr_count) { /* avoid mm_alloc(pool, 0); etc. */
 			return d - data;
@@ -78,7 +80,7 @@ static int rdataset_materialize(knot_rdataset_t * restrict rds, const void * con
 		return kr_error(ENOMEM);
 	}
 	/* Construct the output, one "RR" at a time. */
-	d = data + 1/*sizeof(rr_count)*/;
+	d = data + KR_CACHE_RR_COUNT_SIZE;
 	knot_rdata_t *d_out = rds->data; /* iterates over the output being materialized */
 	for (int i = 0; i < rds->rr_count; ++i) {
 		uint16_t len;
diff --git a/lib/cache/impl.h b/lib/cache/impl.h
index 72e33704a..60ee9fe5d 100644
--- a/lib/cache/impl.h
+++ b/lib/cache/impl.h
@@ -156,10 +156,13 @@ int32_t get_new_ttl(const struct entry_h *entry, const struct kr_query *qry,
 
 /* RRset (de)materialization; implementation in ./entry_rr.c */
 
+/** Size of the RR count field */
+#define KR_CACHE_RR_COUNT_SIZE sizeof(uint16_t)
+
 /** Compute size of dematerialized rdataset.  NULL is accepted as empty set. */
 static inline int rdataset_dematerialize_size(const knot_rdataset_t *rds)
 {
-	return 1/*sizeof(rr_count)*/ + (rds
+	return KR_CACHE_RR_COUNT_SIZE + (rds
 		? knot_rdataset_size(rds) - 4 * rds->rr_count /*TTLs*/
 		: 0);
 }
diff --git a/lib/cache/nsec1.c b/lib/cache/nsec1.c
index ce1c39f0d..c3e3e2208 100644
--- a/lib/cache/nsec1.c
+++ b/lib/cache/nsec1.c
@@ -216,7 +216,7 @@ static const char * find_leq_NSEC1(struct kr_cache *cache, const struct kr_query
 	}
 	/* We know it starts before sname, so let's check the other end.
 	 * 1. construct the key for the next name - kwz_hi. */
-	const knot_dname_t *next = eh->data + 3; /* it's *full* name ATM */
+	const knot_dname_t *next = eh->data + 2 * KR_CACHE_RR_COUNT_SIZE; /* it's *full* name ATM */
 	if (!eh->data[0]) {
 		assert(false);
 		return "ERROR";