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 98d611c81ec30e10031adc18049c3edf14f02fe9..638124407d9c0a2357cc5ca8243444c2b0d78624 100644 (file)
--- 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 44516e4bc1e2ede04cf504082f3866ab7e7f4191..398af594ba8f07e0fdea86e1bf7a6c89f5d1ae3d 100644 (file)
--- 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 53de0fa55b62e6b89e9459c8792b680e7d3af3b3..ba199477dc4ee9a5271c6636445d7f7e0e4d0a96 100644 (file)
--- 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 6f5de26279ce1f390ba7fcb1fcfecb091e70856d..7d9df64efb22ce77f8f9225b2a485deb404002e5 100644 (file)
--- 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 aabf7a3f58f6843e3b6cf92836e02420ee1446de..dbcc5064d6f1471c9328b6ffcd87f7ed39b87d48 100644 (file)
--- 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 0617cce53cf858461d924ca0c9da17cce6678ec7..9bceeb10859140047b5334c27b2ac7757025b7cb 100644 (file)
--- 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 9b83741f0890d521ed973446ef5793ed1394ac82..89c1b054c0950a53341e3e1fcca197e4aa717fd3 100644 (file)
--- 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 99c2aa3c5e72f0f068b8c414a402e89f129f00f7..e961033d992705a16c9852b31fe880a23c1ab806 100644 (file)
--- 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 c9d55f4e95bcb3fb976a2972e7f53ffb000f2b5b..23f02f28b74615abdf9fc07f53c653f5c1392d74 100644 (file)
--- 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 2263ac63e83b5735881d2f888bb5a8fccf705c7d..4d6b0ff3df0cd49eb6744bedc027083643131313 100644 (file)
--- 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