fix: Lua m.setvar() returned -1, handing the script stack garbage

src/engine/lua.cc

@@ -325,9 +325,6 @@
     Transaction *t(NULL);
     const char *var_value(NULL);
     const char *var_name(NULL);
-    std::string vname;
-    std::string collection;
-    std::string variableName;
     int nargs = lua_gettop(L);
     size_t pos;
     void *z(NULL);
@@ -337,21 +334,23 @@
     t = reinterpret_cast<Transaction *>(z);
 
 
+    /* A lua_CFunction returns the number of values it pushed, so the
+     * error paths return 0 (the script gets nil), never a negative value.
+     */
     if (nargs != 2) {
         ms_dbg_a(t, 8,
-            "m.setvar: Failed m.setvar funtion must has 2 arguments");
-        return -1;
+            "m.setvar: Failed m.setvar function must have 2 arguments");
+        return 0;
     }
+    /* luaL_checkstring() raises a Lua error (longjmp) on a non-string
+     * argument, so call it before any C++ object with a destructor exists.
+     */
     var_value = luaL_checkstring(L, 2);
     var_name = luaL_checkstring(L, 1);
 
-    lua_pop(L, 2);
-
-    if (var_value == NULL || var_name == NULL) {
-        return -1;
-    }
-
-    vname.assign(var_name);
+    std::string vname(var_name);
+    std::string collection;
+    std::string variableName;
     pos = vname.find(".");
     if (pos != std::string::npos) {
         collection = std::string(vname, 0, pos);
@@ -363,7 +362,7 @@
         ms_dbg_a(t, 8,
             "m.setvar: Must specify a collection using dot character" \
             " - ie m.setvar(tx.myvar,mydata)");
-        return -1;
+        return 0;
     }
 
     if (collection == "TX") {
@@ -403,6 +402,10 @@
             var_value);
 
     }
+    else {
+        ms_dbg_a(t, 8, "m.setvar: Unknown collection: "
+            + utils::string::toHexIfNeeded(collection) + ".");
+    }
     return 0;
 }
 

test/test-cases/data/setvar-error-nodot.lua

@@ -0,0 +1,5 @@
+function main()
+    local r = m.setvar("novar", "x");
+    m.log(4, "m.setvar() without a collection returned: " .. tostring(r));
+    return nil;
+end

test/test-cases/data/setvar-error-onearg.lua

@@ -0,0 +1,5 @@
+function main()
+    local r = m.setvar("tx.a");
+    m.log(4, "m.setvar() with one argument returned: " .. tostring(r));
+    return nil;
+end

test/test-cases/data/setvar-error-table.lua

@@ -0,0 +1,5 @@
+function main()
+    m.setvar("tx.a", {});
+    m.log(4, "not reached");
+    return nil;
+end

test/test-cases/data/setvar-error-unknown-collection.lua

@@ -0,0 +1,5 @@
+function main()
+    local r = m.setvar("FOO.bar", "x");
+    m.log(4, "m.setvar() with an unknown collection returned: " .. tostring(r));
+    return nil;
+end

test/test-cases/data/setvar-tx.lua

@@ -0,0 +1,6 @@
+function main()
+    local r = m.setvar("tx.a", "x");
+    m.log(4, "m.setvar() with TX returned: " .. tostring(r));
+    m.log(4, "tx.a is now: " .. tostring(m.getvar("tx.a")));
+    return nil;
+end

test/test-cases/regression/collection-lua.json

@@ -310,5 +310,220 @@
       "SecRuleEngine On",
       "SecRuleScript test-cases/data/match-getvars-args.lua \"id:2,phase:2,deny,status:403\""
     ]
+  },
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "title": "Testing LUA :: m.setvar without a collection returns nil",
+    "resource": "lua",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 123
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "localhost",
+        "User-Agent": "My sweet little browser",
+        "Accept": "*/*",
+        "Content-Length": "0"
+      },
+      "uri": "/whee?res=1",
+      "method": "GET",
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "8"
+      },
+      "body": [
+        "no need."
+      ]
+    },
+    "expected": {
+      "debug_log": "m.setvar: Must specify a collection using dot character.*m.setvar\\(\\) without a collection returned: nil",
+      "http_code": 200
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecRuleScript test-cases/data/setvar-error-nodot.lua \"id:1,pass\""
+    ]
+  },
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "title": "Testing LUA :: m.setvar with one argument returns nil",
+    "resource": "lua",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 123
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "localhost",
+        "User-Agent": "My sweet little browser",
+        "Accept": "*/*",
+        "Content-Length": "0"
+      },
+      "uri": "/whee?res=1",
+      "method": "GET",
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "8"
+      },
+      "body": [
+        "no need."
+      ]
+    },
+    "expected": {
+      "debug_log": "m.setvar: Failed m.setvar function must have 2 arguments.*m.setvar\\(\\) with one argument returned: nil",
+      "http_code": 200
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecRuleScript test-cases/data/setvar-error-onearg.lua \"id:1,pass\""
+    ]
+  },
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "title": "Testing LUA :: m.setvar with an unknown collection returns nil",
+    "resource": "lua",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 123
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "localhost",
+        "User-Agent": "My sweet little browser",
+        "Accept": "*/*",
+        "Content-Length": "0"
+      },
+      "uri": "/whee?res=1",
+      "method": "GET",
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "8"
+      },
+      "body": [
+        "no need."
+      ]
+    },
+    "expected": {
+      "debug_log": "m.setvar: Unknown collection: FOO\\..*m.setvar\\(\\) with an unknown collection returned: nil",
+      "http_code": 200
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecRuleScript test-cases/data/setvar-error-unknown-collection.lua \"id:1,pass\""
+    ]
+  },
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "title": "Testing LUA :: m.setvar sets a TX variable and returns nil",
+    "resource": "lua",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 123
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "localhost",
+        "User-Agent": "My sweet little browser",
+        "Accept": "*/*",
+        "Content-Length": "0"
+      },
+      "uri": "/whee?res=1",
+      "method": "GET",
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "8"
+      },
+      "body": [
+        "no need."
+      ]
+    },
+    "expected": {
+      "debug_log": "m.setvar\\(\\) with TX returned: nil.*tx.a is now: x",
+      "http_code": 200
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecRuleScript test-cases/data/setvar-tx.lua \"id:1,pass\""
+    ]
+  },
+  {
+    "enabled": 1,
+    "version_min": 300000,
+    "title": "Testing LUA :: m.setvar with a non-string value fails the script",
+    "resource": "lua",
+    "client": {
+      "ip": "200.249.12.31",
+      "port": 123
+    },
+    "server": {
+      "ip": "200.249.12.31",
+      "port": 80
+    },
+    "request": {
+      "headers": {
+        "Host": "localhost",
+        "User-Agent": "My sweet little browser",
+        "Accept": "*/*",
+        "Content-Length": "0"
+      },
+      "uri": "/whee?res=1",
+      "method": "GET",
+      "body": [
+        ""
+      ]
+    },
+    "response": {
+      "headers": {
+        "Content-Length": "8"
+      },
+      "body": [
+        "no need."
+      ]
+    },
+    "expected": {
+      "debug_log": "Failed to execute lua script: .*setvar-error-table.lua \\(main\\) - .*bad argument #2 to 'setvar'",
+      "http_code": 200
+    },
+    "rules": [
+      "SecRuleEngine On",
+      "SecRuleScript test-cases/data/setvar-error-table.lua \"id:1,pass\""
+    ]
   }
 ]