From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Tue, 16 Mar 2021 09:39:50 +0000 (+0100)
Subject: utils/cache_gc: fix crashes/assertions on RTT entries
X-Git-Tag: v5.3.1~8^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=db85da95bed119601dee4e1ffb3fbec93081fa3d;p=thirdparty%2Fknot-resolver.git

utils/cache_gc: fix crashes/assertions on RTT entries

I missed some parts when finishing this.  I should've tested it better.
GC would hit assertions or NULL dereferences when removing entries,
and eventually that would lead to cache overflowing (and getting
cleared).
---

diff --git a/NEWS b/NEWS
index 5fba50776..8ddd35ae7 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,7 @@ Improvements
 Bugfixes
 --------
 - dnstap module: don't break request resolution on dnstap errors (!1147)
+- cache garbage collector: fix crashes introduced in 5.3.0 (!1153)
 
 
 Knot Resolver 5.3.0 (2021-02-25)
diff --git a/utils/cache_gc/db.c b/utils/cache_gc/db.c
index 31ff14767..1acd37e68 100644
--- a/utils/cache_gc/db.c
+++ b/utils/cache_gc/db.c
@@ -78,8 +78,10 @@ const uint16_t *kr_gc_key_consistent(knot_db_val_t key)
 	} else {
 		/* find the first double zero in the key */
 		for (i = 2; kd[i - 1] || kd[i - 2]; ++i) {
-			if (i >= key.len)
+			if (i >= key.len) {
+				// TODO: assert(!EINVAL) -> kr_assume()
 				return NULL;
+			}
 		}
 	}
 	// the next character can be used for classification
@@ -94,8 +96,10 @@ const uint16_t *kr_gc_key_consistent(knot_db_val_t key)
 		return &NSEC1;
 	case '3':
 		return &NSEC3;
-	case 'S': // we don't GC the rtt_state entries, at least for now
+	case 'S': // the rtt_state entries are considered inconsistent, at least for now
+		return NULL;
 	default:
+		assert(!EINVAL);
 		return NULL;
 	}
 }
diff --git a/utils/cache_gc/db.h b/utils/cache_gc/db.h
index 9d1bef51b..15d26caa1 100644
--- a/utils/cache_gc/db.h
+++ b/utils/cache_gc/db.h
@@ -21,6 +21,11 @@ typedef int (*kr_gc_iter_callback)(const knot_db_val_t * key,
 int kr_gc_cache_iter(knot_db_t * knot_db, const  kr_cache_gc_cfg_t *cfg,
 			kr_gc_iter_callback callback, void *ctx);
 
+/** Return RR type corresponding to the key or NULL.
+ *
+ * NULL is returned on unexpected values (those also trigger assertion)
+ * and on other kinds of data in cache (e.g. struct rtt_state).
+ */
 const uint16_t *kr_gc_key_consistent(knot_db_val_t key);
 
 /** Printf a *binary* string in a human-readable way. */
diff --git a/utils/cache_gc/kr_cache_gc.c b/utils/cache_gc/kr_cache_gc.c
index 4d25c8f3f..3843d5ca6 100644
--- a/utils/cache_gc/kr_cache_gc.c
+++ b/utils/cache_gc/kr_cache_gc.c
@@ -277,8 +277,8 @@ int kr_cache_gc(kr_cache_gc_cfg_t *cfg, kr_cache_gc_state_t **state)
 		case KNOT_EOK:
 			deleted_records++;
 			const uint16_t *entry_type = kr_gc_key_consistent(**i);
-			assert(entry_type != NULL);
-			rrtypelist_add(&deleted_rrtypes, *entry_type);
+			if (entry_type != NULL) // some "inconsistent" entries are OK
+				rrtypelist_add(&deleted_rrtypes, *entry_type);
 			break;
 		case KNOT_ENOENT:
 			already_gone++;