NEWS

@@ -15,10 +15,13 @@ Improvements
 - lua: remove dependency on lua-socket and lua-sec, used lua-http and cqueues (#512, #521, !894)
 - net.listen(): allow binding to non-local address with freebind option (!898)
 - cache: pre-allocate the file to avoid SIGBUS later (not macOS; !917, #525)
+- lua: be stricter around nonsense returned from modules (!901)
 
 Bugfixes
 --------
 - correctly use EDNS(0) padding in failed answers (!921)
+- policy and daf modules: fix postrules and reroute rules (!901)
+- renumber module: don't accidentally zero-out request's .state (!901)
 
 
 Knot Resolver 4.3.0 (2019-12-04)

daemon/ffimodule.c

@@ -65,26 +65,6 @@ static int l_ffi_defer(lua_State *L)
 	return uv_idle_start(check, l_ffi_resume_cb);
 }
 
-/** @internal Helper for calling the entrypoint, for kr_module functions. */
-static int l_ffi_call_mod(lua_State *L, int argc)
-{
-	int status = lua_pcall(L, argc, 1, 0);
-	if (status != 0) {
-		kr_log_error("error: %s\n", lua_tostring(L, -1));
-		lua_pop(L, 1);
-		return kr_error(EIO);
-	}
-	if (lua_isnumber(L, -1)) { /* Return code */
-		status = lua_tointeger(L, -1);
-	} else if (lua_isthread(L, -1)) { /* Continuations */
-		/* TODO: unused, possibly in a bad shape.  Meant KR_STATE_YIELD? */
-		assert(!ENOTSUP);
-		status = l_ffi_defer(lua_tothread(L, -1));
-	}
-	lua_pop(L, 1);
-	return status;
-}
-
 /** Common part of calling modname.(de)init in lua.
  * The function to call should be on top of the stack and it gets popped. */
 static int l_ffi_modcb(lua_State *L, struct kr_module *module)
@@ -141,9 +121,40 @@ static int l_ffi_call_layer(kr_layer_t *ctx, int slot_ix)
 	 * lua (full) userdata, as that's relatively expensive (GC-allocated).
 	 * Performance: copying isn't ideal, but it's not visible in profiles. */
 	memcpy(&kr_layer_t_static, ctx, sizeof(*ctx));
-	const int ret = l_ffi_call_mod(L, 1);
-	/* The return codes are mixed at this point.  We need to return KR_STATE_* */
-	return ret < 0 ? KR_STATE_FAIL : ret;
+
+	int ret = lua_pcall(L, 1, 1, 0);
+	/* Handle result of the pcall.
+	 * Default state: ctx->req->state seems safer than ctx->state,
+	 * in case the pcall touched req->state. */
+	int state = ctx->req->state;
+	if (ret) { /* Exception or another lua problem. */
+		state = KR_STATE_FAIL;
+		kr_log_error("error: %s\n", lua_tostring(L, -1));
+
+	} else if (lua_isnumber(L, -1)) { /* Explicitly returned state. */
+		state = lua_tointeger(L, -1);
+		if (!kr_state_consistent(state)) {
+			kr_log_error("error: nonsense state returned from lua module layer: %d\n",
+					state);
+			state = KR_STATE_FAIL;
+		}
+
+	} else if (lua_isnil(L, -1)) { /* Don't change state. */
+
+	} else if (lua_isthread(L, -1)) { /* Continuations */
+		/* TODO: unused, possibly in a bad shape.  Meant KR_STATE_YIELD? */
+		assert(!ENOTSUP);
+		if (l_ffi_defer(lua_tothread(L, -1)) != 0)
+			state = KR_STATE_FAIL;
+
+	} else { /* Nonsense returned. */
+		state = KR_STATE_FAIL;
+		kr_log_error("error: nonsense returned from lua module layer: %s\n",
+				lua_tostring(L, -1));
+		/* Unfortunately we can't easily get name of the module/function here. */
+	}
+	lua_pop(L, 1);
+	return state;
 }
 
 static int l_ffi_layer_begin(kr_layer_t *ctx)

lib/layer.h

@@ -59,6 +59,12 @@ enum kr_layer_state {
 	KR_STATE_YIELD   = 1 << 4, /*!< Paused, waiting for a sub-query. */
 };
 
+/** Check that a kr_layer_state makes sense.  We're not very strict ATM. */
+static inline bool kr_state_consistent(enum kr_layer_state s)
+{
+	return s >= 0 && s < (1 << 5);
+}
+
 /* Forward declarations. */
 struct kr_layer_api;
 
@@ -72,7 +78,10 @@ typedef struct kr_layer {
 	bool is_stream;       /*!< In glue for checkout layer it's used to pass the parameter. */
 } kr_layer_t;
 
-/** Packet processing module API.  All functions return the new kr_layer_state. */
+/** Packet processing module API.  All functions return the new kr_layer_state.
+ *
+ * Lua modules are allowed to return nil/nothing, meaning the state shall not change.
+ */
 struct kr_layer_api {
       	/** Start of processing the DNS request. */
 	int (*begin)(kr_layer_t *ctx);

modules/daf/test.integr/deckard.yaml

+programs:
+- name: kresd
+  binary: kresd
+  additional:
+    - -f
+    - "1"
+  templates:
+    - modules/daf/test.integr/kresd_config.j2
+    - tests/integration/hints_zone.j2
+  configs:
+    - config
+    - hints

modules/daf/test.integr/kresd_config.j2

+{% raw %}
+-- make sure DNSSEC is turned off for tests
+trust_anchors.remove('.')
+
+-- Disable RFC5011 TA update
+if ta_update then
+        modules.unload('ta_update')
+end
+
+-- Disable RFC8145 signaling, scenario doesn't provide expected answers
+if ta_signal_query then
+        modules.unload('ta_signal_query')
+end
+
+-- Disable RFC8109 priming, scenario doesn't provide expected answers
+if priming then
+        modules.unload('priming')
+end
+
+-- Disable this module because it make one priming query
+if detect_time_skew then
+        modules.unload('detect_time_skew')
+end
+
+modules.load('hints > iterate')
+modules.load('daf')
+
+hints['hints.net.'] = '192.0.2.1'
+
+daf.add('src = 127.0.0.0/8 reroute 192.0.2.1-192.0.2.101')
+
+policy.add(policy.suffix(policy.PASS, {todname('test.')}))
+
+_hint_root_file('hints')
+cache.size = 2*MB
+verbose(true)
+{% endraw %}
+
+net = { '{{SELF_ADDR}}' }
+
+
+{% if QMIN == "false" %}
+option('NO_MINIMIZE', true)
+{% else %}
+option('NO_MINIMIZE', false)
+{% endif %}
+
+
+-- Self-checks on globals
+assert(help() ~= nil)
+assert(worker.id ~= nil)
+-- Self-checks on facilities
+assert(cache.count() == 0)
+assert(cache.stats() ~= nil)
+assert(cache.backends() ~= nil)
+assert(worker.stats() ~= nil)
+assert(net.interfaces() ~= nil)
+-- Self-checks on loaded stuff
+assert(net.list()[1].transport.ip == '{{SELF_ADDR}}')
+assert(#modules.list() > 0)
+-- Self-check timers
+ev = event.recurrent(1 * sec, function (ev) return 1 end)
+event.cancel(ev)
+ev = event.after(0, function (ev) return 1 end)

modules/daf/test.integr/module_daf.rpl

+; config options
+;	target-fetch-policy: "0 0 0 0 0"
+;	module-config: "iterator"
+;	name: "."
+	stub-addr: 193.0.14.129 	# K.ROOT-SERVERS.NET.
+CONFIG_END
+
+SCENARIO_BEGIN Test DNS Application Firewall
+
+STEP 11 QUERY
+ENTRY_BEGIN
+REPLY RD
+SECTION QUESTION
+hints.net. IN A
+ENTRY_END
+
+; test rewrite rule applies to hints
+STEP 12 CHECK_ANSWER
+ENTRY_BEGIN
+MATCH all
+REPLY QR RD RA NOERROR
+SECTION QUESTION
+hints.net. IN A
+SECTION ANSWER
+hints.net. IN A	192.0.2.101
+ENTRY_END
+
+
+SCENARIO_END

modules/meson.build

@@ -30,6 +30,7 @@ config_tests += [
 
 integr_tests += [
   ['bogus_log', join_paths(meson.current_source_dir(), 'bogus_log', 'test.integr')],
+  ['daf', join_paths(meson.current_source_dir(), 'daf', 'test.integr')],
   ['rebinding', join_paths(meson.current_source_dir(), 'rebinding', 'test.integr')],
   ['serve_stale', join_paths(meson.current_source_dir(), 'serve_stale', 'test.integr')],
   # NOTE: ta_update may pass in cases when it should fail due to race conditions

modules/policy/policy.lua

@@ -789,7 +789,7 @@ end
 -- as a dependency chain, e.g. r1,r2,r3 -> r3(r2(r1(state)))
 policy.layer = {
 	begin = function(state, req)
-		-- Don't act on "resolved" cases.
+		-- Don't act on "finished" cases.
 		if bit.band(state, bit.bor(kres.FAIL, kres.DONE)) ~= 0 then return state end
 		local qry = req:current()
 		return policy.evaluate(policy.rules, req, qry, state)
@@ -800,8 +800,8 @@ policy.layer = {
 	finish = function(state, req)
 		-- Optimization for the typical case
 		if #policy.postrules == 0 then return state end
-		-- Don't act on "resolved" cases.
-		if bit.band(state, bit.bor(kres.FAIL, kres.DONE)) ~= 0 then return state end
+		-- Don't act on failed cases.
+		if bit.band(state, kres.FAIL) ~= 0 then return state end
 		return policy.evaluate(policy.postrules, req, req:current(), state) or state
 	end
 }

modules/renumber/renumber.lua

 -- Module interface
 local ffi = require('ffi')
-local prefixes = {}
+local prefixes_global = {}
 
 -- Create subnet prefix rule
 local function matchprefix(subnet, addr)
@@ -28,7 +28,7 @@ end
 
 -- Add subnet prefix rewrite rule
 local function add_prefix(subnet, addr)
-	table.insert(prefixes, matchprefix(subnet, addr))
+	table.insert(prefixes_global, matchprefix(subnet, addr))
 end
 
 -- Match IP against given subnet or record owner
@@ -61,7 +61,7 @@ local function renumber_record(tbl, rr)
 end
 
 -- Renumber addresses based on config
-local function rule()
+local function rule(prefixes)
 	return function (state, req)
 		if state == kres.FAIL then return state end
 		local pkt = req.answer
@@ -82,7 +82,7 @@ local function rule()
 			end
 		end
 		-- If not rewritten, chain action
-		if not changed then return end
+		if not changed then return state end
 		-- Replace section if renumbering
 		local qname = pkt:qname()
 		local qclass = pkt:qclass()
@@ -119,7 +119,7 @@ end
 
 -- Layers
 M.layer = {
-	finish = rule(),
+	finish = rule(prefixes_global),
 }
 
 return M

modules/view/view.lua

@@ -106,7 +106,7 @@ end
 -- @function Module layers
 view.layer = {
 	begin = function(state, req)
-		-- Don't act on "resolved" cases.
+		-- Don't act on "finished" cases.
 		if bit.band(state, bit.bor(kres.FAIL, kres.DONE)) ~= 0 then return state end
 
 		evaluate(state, req)