From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Thu, 23 Jan 2020 16:30:33 +0000 (+0100)
Subject: lua: be stricter around nonsense returned from modules
X-Git-Tag: v5.0.0~2^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7423ac6bfae2d7c2d24f50433f565a0127f85906;p=thirdparty%2Fknot-resolver.git

lua: be stricter around nonsense returned from modules

Well, the part about giving semantics to nil actually
decreases strictness.
---

diff --git a/daemon/ffimodule.c b/daemon/ffimodule.c
index c1cb50e98..7e55b09f3 100644
--- a/daemon/ffimodule.c
+++ b/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)
diff --git a/lib/layer.h b/lib/layer.h
index 5fbd9df2f..823db83ec 100644
--- a/lib/layer.h
+++ b/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);