From 1aa326d682a3f8df90a6b652635a7773282e05cc Mon Sep 17 00:00:00 2001
From: Matthew Zipkin <pinheadmz@gmail.com>
Date: Tue, 1 Mar 2022 10:32:19 -0500
Subject: [PATCH] dns: prevent memory leak when reading invalid record data

Before this commmit, if record data was invalid and ..._read() failed,
we would `goto fail` which would call hsk_dns_rrs_uninit(). However,
uninit() only frees rr objects if the rrset has a size > 0 and only
frees that many rr objects.

By calling hsk_dns_rrs_push() BEFORE ..._read() we increment that
size value and that ensures that if there is a failure, the rr
will be freed.

The rr is already allocated and pushing its pointer into the rrset
before we write its data doesn't affect anything else.
---
 src/dns.c        | 18 ++++++++++--------
 test/hnsd-test.c |  2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/dns.c b/src/dns.c
index 845368f8..c3ea449a 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -404,10 +404,10 @@ hsk_dns_msg_read(uint8_t **data, size_t *data_len, hsk_dns_msg_t *msg) {
     if (!qs)
       goto fail;
 
+    hsk_dns_rrs_push(&msg->qd, qs);
+
     if (!hsk_dns_qs_read(data, data_len, &dmp, qs))
       goto fail;
-
-    hsk_dns_rrs_push(&msg->qd, qs);
   }
 
   for (i = 0; i < ancount; i++) {
@@ -419,10 +419,10 @@ hsk_dns_msg_read(uint8_t **data, size_t *data_len, hsk_dns_msg_t *msg) {
     if (!rr)
       goto fail;
 
+    hsk_dns_rrs_push(&msg->an, rr);
+
     if (!hsk_dns_rr_read(data, data_len, &dmp, rr))
       goto fail;
-
-    hsk_dns_rrs_push(&msg->an, rr);
   }
 
   for (i = 0; i < nscount; i++) {
@@ -434,10 +434,10 @@ hsk_dns_msg_read(uint8_t **data, size_t *data_len, hsk_dns_msg_t *msg) {
     if (!rr)
       goto fail;
 
+    hsk_dns_rrs_push(&msg->ns, rr);
+
     if (!hsk_dns_rr_read(data, data_len, &dmp, rr))
       goto fail;
-
-    hsk_dns_rrs_push(&msg->ns, rr);
   }
 
   for (i = 0; i < arcount; i++) {
@@ -449,6 +449,8 @@ hsk_dns_msg_read(uint8_t **data, size_t *data_len, hsk_dns_msg_t *msg) {
     if (!rr)
       goto fail;
 
+    hsk_dns_rrs_push(&msg->ar, rr);
+
     if (!hsk_dns_rr_read(data, data_len, &dmp, rr))
       goto fail;
 
@@ -467,13 +469,13 @@ hsk_dns_msg_read(uint8_t **data, size_t *data_len, hsk_dns_msg_t *msg) {
       msg->edns.rd = opt->rd;
       msg->code |= msg->edns.code << 4;
 
+      hsk_dns_rr_t *popped = hsk_dns_rrs_pop(&msg->ar);
+      assert(popped == rr);
       free(rr->rd);
       free(rr);
 
       continue;
     }
-
-    hsk_dns_rrs_push(&msg->ar, rr);
   }
 
   return true;
diff --git a/test/hnsd-test.c b/test/hnsd-test.c
index 6f267704..1ae3a76f 100644
--- a/test/hnsd-test.c
+++ b/test/hnsd-test.c
@@ -298,7 +298,7 @@ test_hsk_dns_msg_write() {
 
     printf("   TYPE:%02d %s\n",record_read_vector.type, record_read_vector.name1);
 
-    uint8_t actual[total_len];
+    uint8_t actual[1024]; // enough for invalid message
     uint8_t *actual_ = (uint8_t *)&actual;
     int written = hsk_dns_msg_write(msg, &actual_);