Merge pull request #2070 from pi-hole/fix/civetweb_lua_issue

Fix Civetweb Lua error reporting issue

.devcontainer/devcontainer.json

@@ -16,7 +16,7 @@
   },
   "mounts": [
     "type=bind,source=/home/${localEnv:USER}/.ssh,target=/root/.ssh,readonly",
-    "type=bind,source=/var/www/html,target=/var/www/html,readonly"
+    "type=bind,source=/var/www/html/admin,target=/var/www/html/admin,readonly"
   ]
 
 }

patch/lua.sh

@@ -2,5 +2,7 @@
 set -e
 
 patch -p1 < patch/lua/0001-add-pihole-library.patch
+patch -p1 < patch/lua/0001-Increase-LUA_IDSIZE-so-that-long-script-filenames-as.patch
+patch -p1 < patch/lua/0001-Add-bundled-script-loading-into-luaL_openlibs-to-mak.patch
 
 echo "ALL PATCHES APPLIED OKAY"

patch/lua/0001-Add-bundled-script-loading-into-luaL_openlibs-to-mak.patch

@@ -0,0 +1,90 @@
+From 0ff00e1c838ec91a31970c2b51a7651954cba3d6 Mon Sep 17 00:00:00 2001
+From: DL6ER <[email protected]>
+Date: Mon, 23 Sep 2024 21:42:21 +0200
+Subject: [PATCH] Add bundled script loading into luaL_openlibs to make them
+ available globally (also in the webserver)
+
+Signed-off-by: DL6ER <[email protected]>
+---
+ src/lua/ftl_lua.h                  |  2 --
+ src/lua/linit.c                    |  6 ++++++
+ src/lua/lua.c                      | 13 +------------
+ 3 files changed, 7 insertions(+), 14 deletions(-)
+
+diff --git a/src/lua/ftl_lua.h b/src/lua/ftl_lua.h
+index d986498a..30bad1f9 100644
+--- a/src/lua/ftl_lua.h
++++ b/src/lua/ftl_lua.h
+@@ -21,8 +21,6 @@ int run_luac(const int argc, char **argv);
+ int lua_main (int argc, char **argv);
+ int luac_main (int argc, char **argv);
+
+-extern int dolibrary (lua_State *L, char *name);
+-
+ void print_embedded_scripts(void);
+ void ftl_lua_init(lua_State *L);
+
+diff --git a/src/lua/linit.c b/src/lua/linit.c
+index 9a5bcfdc..787865c0 100644
+--- a/src/lua/linit.c
++++ b/src/lua/linit.c
+@@ -8,6 +8,10 @@
+ #define linit_c
+ #define LUA_LIB
+
++/** Pi-hole modification **/
++#include "ftl_lua.h"
++/**************************/
++
+ /*
+ ** If you embed Lua in your program and need to open the standard
+ ** libraries, call luaL_openlibs in your program. If you need a
+@@ -64,5 +68,7 @@ LUALIB_API void luaL_openlibs (lua_State *L) {
+     luaL_requiref(L, lib->name, lib->func, 1);
+     lua_pop(L, 1);  /* remove lib */
+   }
++  // Load and enable libraries bundled with Pi-hole
++  ftl_lua_init(L);
+ }
+
+diff --git a/src/lua/lua.c b/src/lua/lua.c
+index 35fb281d..111a1b2b 100644
+--- a/src/lua/lua.c
++++ b/src/lua/lua.c
+@@ -20,10 +20,6 @@
+ #include "lauxlib.h"
+ #include "lualib.h"
+
+-/** Pi-hole modification **/
+-#include "ftl_lua.h"
+-/**************************/
+-
+
+ #if !defined(LUA_PROGNAME)
+ #define LUA_PROGNAME		"lua"
+@@ -218,9 +214,7 @@ static int dostring (lua_State *L, const char *s, const char *name) {
+ ** If there is no explicit modname and globname contains a '-', cut
+ ** the suffix after '-' (the "version") to make the global name.
+ */
+-/************** Pi-hole modification ***************/
+-int dolibrary (lua_State *L, char *globname) {
+-/***************************************************/
++static int dolibrary (lua_State *L, char *globname) {
+   int status;
+   char *suffix = NULL;
+   char *modname = strchr(globname, '=');
+@@ -655,11 +649,6 @@ static int pmain (lua_State *L) {
+       return 0;  /* error running LUA_INIT */
+   }
+
+-  /************** Pi-hole modification ***************/
+-  // Load and enable libraries bundled with Pi-hole
+-  ftl_lua_init(L);
+-  /***************************************************/
+-
+   if (!runargs(L, argv, optlim))  /* execute arguments -e and -l */
+     return 0;  /* something failed */
+   if (script > 0) {  /* execute main script (if there is one) */
+-- 
+2.34.1
+

patch/lua/0001-Increase-LUA_IDSIZE-so-that-long-script-filenames-as.patch

@@ -0,0 +1,27 @@
+From 835933f8501e517a781b380b8cfafa656adc6fa7 Mon Sep 17 00:00:00 2001
+From: DL6ER <[email protected]>
+Date: Mon, 23 Sep 2024 13:25:34 +0200
+Subject: [PATCH] Increase LUA_IDSIZE so that long script filenames as well as
+ long script lines fit into the error logging buffer
+
+Signed-off-by: DL6ER <[email protected]>
+---
+ src/lua/luaconf.h | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/src/lua/luaconf.h b/src/lua/luaconf.h
+index 33bb580d..dacc5221 100644
+--- a/src/lua/luaconf.h
++++ b/src/lua/luaconf.h
+@@ -765,7 +765,7 @@
+ ** of a function in debug information.
+ ** CHANGE it if you want a different size.
+ */
+-#define LUA_IDSIZE	60
++#define LUA_IDSIZE	256
+
+
+ /*
+-- 
+2.34.1
+

src/lua/ftl_lua.h

@@ -21,8 +21,6 @@ int run_luac(const int argc, char **argv);
 int lua_main (int argc, char **argv);
 int luac_main (int argc, char **argv);
 
-extern int dolibrary (lua_State *L, char *name);
-
 void print_embedded_scripts(void);
 void ftl_lua_init(lua_State *L);
 

src/lua/linit.c

@@ -8,6 +8,10 @@
 #define linit_c
 #define LUA_LIB
 
+/** Pi-hole modification **/
+#include "ftl_lua.h"
+/**************************/
+
 /*
 ** If you embed Lua in your program and need to open the standard
 ** libraries, call luaL_openlibs in your program. If you need a
@@ -64,5 +68,7 @@ LUALIB_API void luaL_openlibs (lua_State *L) {
     luaL_requiref(L, lib->name, lib->func, 1);
     lua_pop(L, 1);  /* remove lib */
   }
+  // Load and enable libraries bundled with Pi-hole
+  ftl_lua_init(L);
 }
 

src/lua/lua.c

@@ -20,10 +20,6 @@
 #include "lauxlib.h"
 #include "lualib.h"
 
-/** Pi-hole modification **/
-#include "ftl_lua.h"
-/**************************/
-
 
 #if !defined(LUA_PROGNAME)
 #define LUA_PROGNAME		"lua"
@@ -218,9 +214,7 @@ static int dostring (lua_State *L, const char *s, const char *name) {
 ** If there is no explicit modname and globname contains a '-', cut
 ** the suffix after '-' (the "version") to make the global name.
 */
-/************** Pi-hole modification ***************/
-int dolibrary (lua_State *L, char *globname) {
-/***************************************************/
+static int dolibrary (lua_State *L, char *globname) {
   int status;
   char *suffix = NULL;
   char *modname = strchr(globname, '=');
@@ -655,11 +649,6 @@ static int pmain (lua_State *L) {
       return 0;  /* error running LUA_INIT */
   }
 
-  /************** Pi-hole modification ***************/
-  // Load and enable libraries bundled with Pi-hole
-  ftl_lua_init(L);
-  /***************************************************/
-
   if (!runargs(L, argv, optlim))  /* execute arguments -e and -l */
     return 0;  /* something failed */
   if (script > 0) {  /* execute main script (if there is one) */

src/lua/luaconf.h

@@ -765,7 +765,7 @@
 ** of a function in debug information.
 ** CHANGE it if you want a different size.
 */
-#define LUA_IDSIZE	60
+#define LUA_IDSIZE	256
 
 
 /*

src/webserver/civetweb/mod_lua.inl

@@ -10,6 +10,8 @@
 #include "civetweb_lua.h"
 #include "civetweb_private_lua.h"
 
+static int
+lua_error_handler(lua_State *L);
 
 #if defined(_WIN32)
 static void *
@@ -670,11 +672,19 @@ run_lsp_kepler(struct mg_connection *conn,
 		/* Syntax error or OOM.
 		 * Error message is pushed on stack. */
 		lua_pcall(L, 1, 0, 0);
-		lua_cry(conn, lua_ok, L, "LSP", "execute"); /* XXX TODO: everywhere ! */
+		lua_cry(conn, lua_ok, L, "LSP Kepler", "execute");
+		lua_error_handler(L);
+		return 1;
 
 	} else {
 		/* Success loading chunk. Call it. */
-		lua_pcall(L, 0, 0, 1);
+		lua_ok = lua_pcall(L, 0, 0, 0);
+		if(lua_ok != LUA_OK)
+		{
+			lua_cry(conn, lua_ok, L, "LSP Kepler", "call");
+			lua_error_handler(L);
+			return 1;
+		}
 	}
 	return 0;
 }
@@ -788,9 +798,18 @@ run_lsp_civetweb(struct mg_connection *conn,
 						/* Syntax error or OOM.
 						 * Error message is pushed on stack. */
 						lua_pcall(L, 1, 0, 0);
+						lua_cry(conn, lua_ok, L, "LSP", "call");
+						lua_error_handler(L);
+						return 1;
 					} else {
 						/* Success loading chunk. Call it. */
-						lua_pcall(L, 0, 0, 1);
+						lua_ok = lua_pcall(L, 0, 0, 0);
+						if(lua_ok != LUA_OK)
+						{
+							lua_cry(conn, lua_ok, L, "LSP", "execute");
+							lua_error_handler(L);
+							return 1;
+						}
 					}
 
 					/* Progress until after the Lua closing tag. */
@@ -2781,18 +2800,39 @@ lua_error_handler(lua_State *L)
 
 	lua_getglobal(L, "mg");
 	if (!lua_isnil(L, -1)) {
-		lua_getfield(L, -1, "write"); /* call mg.write() */
+		/* Write the error message to the error log */
+		lua_getfield(L, -1, "write");
 		lua_pushstring(L, error_msg);
 		lua_pushliteral(L, "\n");
-		lua_call(L, 2, 0);
-		IGNORE_UNUSED_RESULT(
-		    luaL_dostring(L, "mg.write(debug.traceback(), '\\n')"));
+		lua_call(L, 2, 0); /* call mg.write(error_msg + \n) */
+		lua_pop(L, 1); /* pop mg */
+
+		/* Get Lua traceback */
+		lua_getglobal(L, "debug");
+		lua_getfield(L, -1, "traceback");
+		lua_call(L, 0, 1); /* call debug.traceback() */
+		lua_remove(L, -2); /* remove debug */
+
+		/* Write the Lua traceback to the error log */
+		lua_getglobal(L, "mg");
+		lua_getfield(L, -1, "write");
+		lua_pushvalue(L, -3); /* push the traceback */
+
+		/* Only print the traceback if it is not empty */
+		if (strcmp(lua_tostring(L, -1), "stack traceback:") != 0) {
+			lua_pushliteral(L, "\n"); /* append a newline */
+			lua_call(L, 2, 0); /* call mg.write(traceback + \n) */
+			lua_pop(L, 2); /* pop mg and traceback */
+		} else {
+			lua_pop(L, 3); /* pop mg, traceback and write */
+		}
+
 	} else {
 		printf("Lua error: [%s]\n", error_msg);
 		IGNORE_UNUSED_RESULT(
 		    luaL_dostring(L, "print(debug.traceback(), '\\n')"));
 	}
-	/* TODO(lsm, low): leave the stack balanced */
+	lua_pop(L, 1); /* pop error message */
 
 	return 0;
 }

src/webserver/lua_web.c

@@ -57,12 +57,7 @@ void free_lua(void)
 
 void init_lua(const struct mg_connection *conn, void *L, unsigned context_flags)
 {
-	// Set onerror handler to print errors to the log
-	if(luaL_dostring(L, "mg.onerror = function(e) mg.cry('Error at ' .. e) end") != LUA_OK)
-	{
-		log_err("Error setting Lua onerror handler: %s", lua_tostring(L, -1));
-		lua_pop(L, 1);
-	}
+	return;
 }
 
 int request_handler(struct mg_connection *conn, void *cbdata)

test/broken_lua.lp

@@ -0,0 +1,4 @@
+<?
+mg.write("Hello, world 1!\n")
+mg.include("broken_lua_2.lp", "r")
+?>

test/broken_lua_2.lp

@@ -0,0 +1,4 @@
+<?
+mg.write("Hello, world 2!\n")
+mg.include("does_not_exist.lp", "r")
+?>

test/run.sh

@@ -23,7 +23,7 @@ done
 rm -rf /etc/pihole /var/log/pihole /dev/shm/FTL-*
 
 # Create necessary directories and files
-mkdir -p /home/pihole /etc/pihole /run/pihole /var/log/pihole /etc/pihole/config_backups
+mkdir -p /home/pihole /etc/pihole /run/pihole /var/log/pihole /etc/pihole/config_backups /var/www/html
 echo "" > /var/log/pihole/FTL.log
 echo "" > /var/log/pihole/pihole.log
 echo "" > /var/log/pihole/webserver.log
@@ -62,6 +62,10 @@ cp test/01-pihole-tests.conf /etc/dnsmasq.d/01-pihole-tests.conf
 # Prepare versions file (read by /api/version)
 cp test/versions /etc/pihole/versions
 
+# Prepare Lua test script
+cp test/broken_lua.lp /var/www/html/broken_lua.lp
+cp test/broken_lua_2.lp /var/www/html/broken_lua_2.lp
+
 # Prepare local powerDNS resolver
 bash test/pdns/setup.sh
 

test/test_suite.bats

@@ -1235,10 +1235,10 @@
   [[ ${lines[0]} == '{"error":{"key":"not_found","message":"Not found","hint":"/api/undefined"},"took":'*'}' ]]
 }
 
-@test "HTTP server responds with normal error 404 to path outside /admin" {
-  run bash -c 'curl -s 127.0.0.1/undefined'
+@test "HTTP server responds with error 404 to path outside /admin" {
+  run bash -c 'curl -sI 127.0.0.1/undefined'
   printf "%s\n" "${lines[@]}"
-  [[ ${lines[0]} == "Error 404: Not Found" ]]
+  [[ ${lines[@]} == *"HTTP/1.1 404 Not Found"* ]]
 }
 
 @test "LUA: Interpreter returns FTL version" {
@@ -1684,6 +1684,25 @@
   [[ ${lines[0]} == "0" ]]
 }
 
+# This test should run before a password it set
+@test "Lua server page is generating proper backtrace" {
+  # Run a page with a syntax error
+  run bash -c 'curl -s 127.0.0.1/broken_lua'
+  printf "%s\n" "${lines[@]}"
+  [[ ${lines[0]} == 'Hello, world 1!' ]]
+  [[ ${lines[1]} == 'Hello, world 2!' ]]
+  [[ ${lines[2]} == '[string "/var/www/html/broken_lua_2.lp"]:4: Cannot include [/var/www/html/does_not_exist.lp]: not found' ]]
+  [[ ${lines[3]} == 'stack traceback:' ]]
+  [[ ${lines[4]} == "	[C]: in field 'include'" ]]
+  [[ ${lines[5]} == '	[string "/var/www/html/broken_lua.lp"]:4: in main chunk' ]]
+  [[ ${lines[6]} == 'aborting' ]]
+  [[ ${lines[7]} == '' ]]
+
+  # Check if the error is logged (-F = fixed string (no regex), -q = quiet)
+  run grep -qF 'LSP Kepler: call failed: runtime error: [string "/var/www/html/broken_lua_2.lp"]:4: Cannot include [/var/www/html/does_not_exist.lp]: not found' /var/log/pihole/webserver.log
+  [[ $status == 0 ]]
+}
+
 @test "API authorization (without password): No login required" {
   run bash -c 'curl -s 127.0.0.1/api/auth'
   printf "%s\n" "${lines[@]}"