From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Fri, 25 Sep 2020 15:59:57 +0000 (+0200)
Subject: lib/resolve kr_request_ensure_answer(): allow it to fail
X-Git-Tag: v5.2.0~9^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpipelines%2F70860;p=thirdparty%2Fknot-resolver.git

lib/resolve kr_request_ensure_answer(): allow it to fail

For now I was too afraid to use "multi-flag" kr_request::state,
so I kept it at _FAIL; anyone can recognize it by NULL answer anyway.

Lua wrapper: using exception was considered but didn't seem good.
I utilized the fact that modules can return nil meaning no state change.
---

diff --git a/lib/layer.h b/lib/layer.h
index 98d611c81..638124407 100644
--- a/lib/layer.h
+++ b/lib/layer.h
@@ -99,7 +99,8 @@ struct kr_layer_api {
 	int (*checkout)(kr_layer_t *ctx, knot_pkt_t *packet, struct sockaddr *dst, int type);
 
 	/** Finalises the answer.
-	 * Last chance to affect what will get into the answer, including EDNS.*/
+	 * Last chance to affect what will get into the answer, including EDNS.
+	 * Not called if the packet is being dropped. */
 	int (*answer_finalize)(kr_layer_t *ctx);
 
 	/** The C module can store anything in here. */
diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c
index 44516e4bc..398af594b 100644
--- a/lib/layer/iterate.c
+++ b/lib/layer/iterate.c
@@ -474,11 +474,15 @@ static int process_authority(knot_pkt_t *pkt, struct kr_request *req)
 	return result;
 }
 
-static void finalize_answer(knot_pkt_t *pkt, struct kr_request *req)
+static int finalize_answer(knot_pkt_t *pkt, struct kr_request *req)
 {
 	/* Finalize header */
 	knot_pkt_t *answer = kr_request_ensure_answer(req);
-	knot_wire_set_rcode(answer->wire, knot_wire_get_rcode(pkt->wire));
+	if (answer) {
+		knot_wire_set_rcode(answer->wire, knot_wire_get_rcode(pkt->wire));
+		req->state = KR_STATE_DONE;
+	}
+	return req->state;
 }
 
 static int unroll_cname(knot_pkt_t *pkt, struct kr_request *req, bool referral, const knot_dname_t **cname_ret)
@@ -677,8 +681,7 @@ static int process_final(knot_pkt_t *pkt, struct kr_request *req,
 			array->at[last_idx] = entry;
 			entry->to_wire = true;
 		}
-		finalize_answer(pkt, req);
-		return KR_STATE_DONE;
+		return finalize_answer(pkt, req);
 	}
 	return kr_ok();
 }
@@ -801,7 +804,7 @@ static int process_answer(knot_pkt_t *pkt, struct kr_request *req)
 		if (state != kr_ok()) {
 			return KR_STATE_FAIL;
 		}
-		finalize_answer(pkt, req);
+		return finalize_answer(pkt, req);
 	} else {
 		/* Answer for sub-query; DS, IP for NS etc.
 		 * It may contains NSEC \ NSEC3 records for
@@ -864,8 +867,7 @@ static int process_stub(knot_pkt_t *pkt, struct kr_request *req)
 		return KR_STATE_FAIL;
 	}
 
-	finalize_answer(pkt, req);
-	return KR_STATE_DONE;
+	return finalize_answer(pkt, req);
 }
 
 
@@ -903,6 +905,7 @@ static int begin(kr_layer_t *ctx)
 	    || (knot_rrtype_is_metatype(qry->stype)
 		    /* && qry->stype != KNOT_RRTYPE_ANY hmm ANY seems broken ATM */)) {
 		knot_pkt_t *ans = kr_request_ensure_answer(ctx->req);
+		if (!ans) return ctx->req->state;
 		knot_wire_set_rcode(ans->wire, KNOT_RCODE_NOTIMPL);
 		return KR_STATE_FAIL;
 	}
@@ -1062,7 +1065,8 @@ static int resolve(kr_layer_t *ctx, knot_pkt_t *pkt)
 	case KNOT_RCODE_NXDOMAIN:
 		break; /* OK */
 	case KNOT_RCODE_YXDOMAIN: /* Basically a successful answer; name just doesn't fit. */
-		kr_request_ensure_answer(req);
+		if (!kr_request_ensure_answer(req))
+			return req->state;
 		knot_wire_set_rcode(req->answer->wire, KNOT_RCODE_YXDOMAIN);
 		break;
 	case KNOT_RCODE_REFUSED:
@@ -1104,7 +1108,8 @@ static int resolve(kr_layer_t *ctx, knot_pkt_t *pkt)
 	}
 
 rrarray_finalize:
-	/* Finish construction of libknot-format RRsets. */
+	/* Finish construction of libknot-format RRsets.
+	 * We do this even if dropping the answer, though it's probably useless. */
 	(void)0;
 	ranked_rr_array_t *selected[] = kr_request_selected(req);
 	for (knot_section_t i = KNOT_ANSWER; i <= KNOT_ADDITIONAL; ++i) {
diff --git a/lib/resolve.c b/lib/resolve.c
index 53de0fa55..ba199477d 100644
--- a/lib/resolve.c
+++ b/lib/resolve.c
@@ -820,16 +820,15 @@ knot_pkt_t * kr_request_ensure_answer(struct kr_request *request)
 		answer->opt_rr = knot_rrset_copy(request->ctx->downstream_opt_rr,
 						 &answer->mm);
 		if (!answer->opt_rr)
-			goto enomem;
+			goto enomem; // answer is on mempool, so "leak" is OK
 		if (knot_pkt_has_dnssec(qs_pkt))
 			knot_edns_set_do(answer->opt_rr);
 	}
 
 	return request->answer;
 enomem:
-	// Consequences of `return NULL` would be way too complicated to handle.
-	kr_log_error("ERROR: irrecoverable out of memory condition in %s\n", __func__);
-	abort();
+	request->state = KR_STATE_FAIL; // TODO: really combine with another flag?
+	return request->answer = NULL;
 }
 
 KR_PURE static bool kr_inaddr_equal(const struct sockaddr *a, const struct sockaddr *b)
@@ -1654,21 +1653,23 @@ int kr_resolve_checkout(struct kr_request *request, const struct sockaddr *src,
 int kr_resolve_finish(struct kr_request *request, int state)
 {
 	request->state = state;
-	kr_request_ensure_answer(request);
-	/* Finalize answer and construct wire-buffer. */
-	ITERATE_LAYERS(request, NULL, answer_finalize);
-	answer_finalize(request);
-
-	/* Defensive style, in case someone has forgotten.
-	 * Beware: non-empty answers do make sense even with SERVFAIL case, etc. */
-	if (request->state != KR_STATE_DONE) {
-		uint8_t *wire = request->answer->wire;
-		switch (knot_wire_get_rcode(wire)) {
-		case KNOT_RCODE_NOERROR:
-		case KNOT_RCODE_NXDOMAIN:
-			knot_wire_clear_ad(wire);
-			knot_wire_clear_aa(wire);
-			knot_wire_set_rcode(wire, KNOT_RCODE_SERVFAIL);
+	/* Finalize answer and construct whole wire-format (unless dropping). */
+	knot_pkt_t *answer = kr_request_ensure_answer(request);
+	if (answer) {
+		ITERATE_LAYERS(request, NULL, answer_finalize);
+		answer_finalize(request);
+
+		/* Defensive style, in case someone has forgotten.
+		 * Beware: non-empty answers do make sense even with SERVFAIL case, etc. */
+		if (request->state != KR_STATE_DONE) {
+			uint8_t *wire = answer->wire;
+			switch (knot_wire_get_rcode(wire)) {
+			case KNOT_RCODE_NOERROR:
+			case KNOT_RCODE_NXDOMAIN:
+				knot_wire_clear_ad(wire);
+				knot_wire_clear_aa(wire);
+				knot_wire_set_rcode(wire, KNOT_RCODE_SERVFAIL);
+			}
 		}
 	}
 
@@ -1677,7 +1678,7 @@ int kr_resolve_finish(struct kr_request *request, int state)
 #ifndef NOVERBOSELOG
 	struct kr_rplan *rplan = &request->rplan;
 	struct kr_query *last = kr_rplan_last(rplan);
-	VERBOSE_MSG(last, "finished: %d, queries: %zu, mempool: %zu B\n",
+	VERBOSE_MSG(last, "finished in state: %d, queries: %zu, mempool: %zu B\n",
 	          request->state, rplan->resolved.len, (size_t) mp_total_size(request->pool.ctx));
 #endif
 
diff --git a/lib/resolve.h b/lib/resolve.h
index 6f5de2627..7d9df64ef 100644
--- a/lib/resolve.h
+++ b/lib/resolve.h
@@ -246,11 +246,11 @@ int kr_resolve_begin(struct kr_request *request, struct kr_context *ctx);
 /**
  * Ensure that request->answer is usable, and return it (for convenience).
  *
- * It can not fail; FIXME: is it worth in the API to abort() instead of return NULL?
+ * It may return NULL, in which case it marks ->state with _FAIL and no answer will be sent.
  * Only use this when it's guaranteed that there will be no delay before sending it.
  * You don't need to call this in places where "resolver knows" that there will be no delay.
  */
-KR_EXPORT __attribute__((returns_nonnull))
+KR_EXPORT
 knot_pkt_t * kr_request_ensure_answer(struct kr_request *request);
 
 /**
diff --git a/modules/policy/README.rst b/modules/policy/README.rst
index aabf7a3f5..dbcc5064d 100644
--- a/modules/policy/README.rst
+++ b/modules/policy/README.rst
@@ -267,6 +267,7 @@ Custom actions
    local ffi = require('ffi')
    local function fake_A_record(state, req)
        local answer = req:ensure_answer()
+       if answer == nil then return nil end
        local qry = req:current()
        if qry.stype ~= kres.type.A then
            return state
diff --git a/modules/policy/policy.lua b/modules/policy/policy.lua
index 0617cce53..9bceeb108 100644
--- a/modules/policy/policy.lua
+++ b/modules/policy/policy.lua
@@ -214,6 +214,7 @@ function policy.ANSWER(rtable, nodata)
 	return function(_, req)
 		local qry = req:current()
 		local answer = req:ensure_answer()
+		if answer == nil then return nil end
 		local data = rtable[qry.stype]
 
 		ffi.C.kr_pkt_make_auth_header(answer)
@@ -255,6 +256,7 @@ local dname_localhost = todname('localhost.')
 local function localhost(_, req)
 	local qry = req:current()
 	local answer = req:ensure_answer()
+	if answer == nil then return nil end
 	ffi.C.kr_pkt_make_auth_header(answer)
 
 	local is_exact = ffi.C.knot_dname_is_equal(qry.sname, dname_localhost)
@@ -287,6 +289,7 @@ local dname_rev4_localhost_apex = todname('127.in-addr.arpa');
 local function localhost_reversed(_, req)
 	local qry = req:current()
 	local answer = req:ensure_answer()
+	if answer == nil then return nil end
 
 	-- classify qry.sname:
 	local is_exact   -- exact dname for localhost
@@ -607,6 +610,7 @@ local function answer_clear(req)
 
 	-- Let's be defensive and clear the answer, too.
 	local pkt = req:ensure_answer()
+	if pkt == nil then return nil end
 	pkt:clear_payload()
 	return pkt
 end
@@ -619,6 +623,7 @@ function policy.DENY_MSG(msg)
 	return function (_, req)
 		-- Write authority information
 		local answer = answer_clear(req)
+		if answer == nil then return nil end
 		ffi.C.kr_pkt_make_auth_header(answer)
 		answer:rcode(kres.rcode.NXDOMAIN)
 		answer:begin(kres.section.AUTHORITY)
@@ -706,12 +711,14 @@ policy.DEBUG_CACHE_MISS = policy.DEBUG_IF(
 policy.DENY = policy.DENY_MSG() -- compatibility with < 2.0
 
 function policy.DROP(_, req)
-	answer_clear(req)
+	local answer = answer_clear(req)
+	if answer == nil then return nil end
 	return kres.FAIL
 end
 
 function policy.REFUSE(_, req)
 	local answer = answer_clear(req)
+	if answer == nil then return nil end
 	answer:rcode(kres.rcode.REFUSED)
 	answer:ad(false)
 	return kres.DONE
@@ -724,6 +731,7 @@ function policy.TC(state, req)
 	end
 
 	local answer = answer_clear(req)
+	if answer == nil then return nil end
 	answer:tc(1)
 	answer:ad(false)
 	return kres.DONE
diff --git a/modules/policy/policy.slice.test.lua b/modules/policy/policy.slice.test.lua
index 9b83741f0..89c1b054c 100644
--- a/modules/policy/policy.slice.test.lua
+++ b/modules/policy/policy.slice.test.lua
@@ -40,6 +40,7 @@ local function sliceaction(index)
 
 		-- refuse query
 		local answer = req:ensure_answer()
+		if answer == nil then return nil end
 		answer:rcode(kres.rcode.REFUSED)
 		answer:ad(false)
 		return kres.DONE
diff --git a/modules/rebinding/rebinding.lua b/modules/rebinding/rebinding.lua
index 99c2aa3c5..e961033d9 100644
--- a/modules/rebinding/rebinding.lua
+++ b/modules/rebinding/rebinding.lua
@@ -72,12 +72,14 @@ end
 local function refuse(req)
 	policy.REFUSE(nil, req)
 	local pkt = req:ensure_answer()
+	if pkt == nil then return nil end
 	pkt:aa(false)
 	pkt:begin(kres.section.ADDITIONAL)
 
 	local msg = 'blocked by DNS rebinding protection'
 	pkt:put('\11explanation\7invalid\0', 10800, pkt:qclass(), kres.type.TXT,
 	string.char(#msg) .. msg)
+	return kres.DONE
 end
 
 -- act on DNS queries which were not answered from cache
@@ -102,14 +104,16 @@ function M.layer.consume(state, req, pkt)
 		Typical example: NS address resolution -> only this NS won't be used
 		but others may still be OK (or we SERVFAIL due to no NS being usable).
 	--]]
-	if qry.parent == nil then refuse(req) end
+	if qry.parent == nil then
+		state = refuse(req)
+	end
 	if verbose() then
 		ffi.C.kr_log_q(qry, 'rebinding',
 		    'blocking blacklisted IP in RR \'%s\' received from IP %s\n',
 		    kres.rr2str(bad_rr),
 		    tostring(kres.sockaddr_t(req.upstream.addr)))
 	end
-	return kres.DONE
+	return state
 end
 
 return M
diff --git a/modules/refuse_nord/refuse_nord.c b/modules/refuse_nord/refuse_nord.c
index c9d55f4e9..23f02f28b 100644
--- a/modules/refuse_nord/refuse_nord.c
+++ b/modules/refuse_nord/refuse_nord.c
@@ -13,14 +13,15 @@ static int refuse_nord_query(kr_layer_t *ctx)
 {
 	struct kr_request *req = ctx->req;
 	uint8_t rd = knot_wire_get_rd(req->qsource.packet->wire);
+	if (rd)
+		return ctx->state;
 
-	if (!rd) {
-		knot_pkt_t *answer = kr_request_ensure_answer(req);
-		knot_wire_set_rcode(answer->wire, KNOT_RCODE_REFUSED);
-		knot_wire_clear_ad(answer->wire);
-		ctx->state = KR_STATE_DONE;
-	}
-
+	knot_pkt_t *answer = kr_request_ensure_answer(req);
+	if (!answer)
+		return ctx->state;
+	knot_wire_set_rcode(answer->wire, KNOT_RCODE_REFUSED);
+	knot_wire_clear_ad(answer->wire);
+	ctx->state = KR_STATE_DONE;
 	return ctx->state;
 }
 
diff --git a/modules/ta_update/ta_update.test.lua b/modules/ta_update/ta_update.test.lua
index 2263ac63e..4d6b0ff3d 100644
--- a/modules/ta_update/ta_update.test.lua
+++ b/modules/ta_update/ta_update.test.lua
@@ -12,6 +12,7 @@ trust_anchors.remove('.')
 counter = 0
 local function counter_func (state, req)
         local answer = req:ensure_answer()
+        if answer == nil then return nil end
         local qry = req:current()
         if answer:qclass() == kres.class.IN
 		and qry.stype == kres.type.DNSKEY