fix: Lua m.setvar() returned -1, handing the script stack garbage by fzipi · Pull Request #3732 · owasp-modsecurity/ModSecurity

fzipi · 2026-11-01T20:12:36Z

what

Fixes three problems in Lua::setvar() (src/engine/lua.cc):

Error paths return 0 instead of -1. This covers the wrong number of arguments and a name with no collection. The script now gets nil.
luaL_checkstring() is called before the std::string locals are constructed. It raises a Lua error with longjmp on a non-string argument, which would skip their destructors. This makes the NULL check after it unreachable, so that check is removed.
The lua_pop(L, 2) is removed. It popped the value argument while var_value still pointed into it. A lua_CFunction's stack is cleared on return anyway.
Typo fixed in the debug message: "funtion must has" is now "function must have".

why

A lua_CFunction returns the number of values it pushed. With -1, Lua returned whatever was on the stack. With the two new test scripts on v3/master, m.setvar("novar", "x") returned the __transaction pointer (userdata: 0x…), and m.setvar("tx.a") returned a stray string.

tests

Two new cases in collection-lua.json, with the scripts setvar-error-nodot.lua and setvar-error-onearg.lua, check that the script gets nil. Both fail on v3/master and pass with this change.
The other Lua regression files pass: action-exec, directive-sec_rule_script, operator-inpectFile and issue-2427.
make check (macOS, Lua 5.5.1, PCRE2): 5139 total, 0 fail.

references

Fixes v3: Lua m.setvar() returns -1 on errors, so the script gets stack garbage #3731
Found while reviewing fix: don't abort on an unknown variable name in Lua m.getvar()/m.getv() #3730

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Invalid calls to set a variable—including missing arguments, missing collection syntax, or non-string values—now return no values instead of an error code.
- Unknown collections are logged and handled without changing a variable. Valid transaction-variable updates continue to work and return no values.
Tests
- Added regression coverage for valid and invalid variable-setting calls.

Lua::setvar() returned -1 on its error paths (wrong number of arguments, no collection in the name). A lua_CFunction returns the number of values it pushed, so Lua returned whatever was on the stack: the script got the __transaction pointer or a stray string. Return 0 so the script gets nil. Also call luaL_checkstring() before the std::string locals exist, since it raises a Lua error with longjmp on a non-string argument, drop the NULL check it makes unreachable, and stop popping the arguments while var_value still points into them. Fixes owasp-modsecurity#3731 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiUJuqXNvvpytiGRiDFyTy

coderabbitai · 2026-11-01T20:13:01Z

No actionable comments were generated in the recent review.

Walkthrough

Lua::setvar now returns zero Lua values on handled errors and validates argument types before constructing local strings. It logs unknown collections. New regression cases cover these paths and successful TX assignment.

Changes: Lua setvar error handling

Layer / File(s)	Summary
Argument checks and return handling `src/engine/lua.cc`	`setvar` checks the argument count and string types before constructing local strings. It returns zero values for invalid arguments and names without a collection separator. It logs unknown collection names.
Regression coverage `test/test-cases/data/setvar-*.lua`, `test/test-cases/regression/collection-lua.json`	Lua scripts and regression cases check missing and invalid arguments, unknown collections, successful TX assignment, and a non-string argument.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

#3737 concerns the same luaL_checkstring() and C++ destructor issue in getvar() and getvars(). Those call sites are not changed here.
#3655 lists Lua setvar return values as one scope item. This change addresses the setvar return handling; its other items remain separate.

Suggested reviewers: airween

Merge Risk: Minimal · up to 9dfe9

Lua m.setvar() now returns no values on handled errors instead of exposing unrelated stack contents, and regression tests cover these cases. No actionable merge-blocking risk remains.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main change: Lua m.setvar() error paths returned -1, which left garbage on the Lua stack. The changeset replaces those -1 returns with zero return values in src/engine/lua.cc and a…
Linked Issues check		Directly linked issue [`#3731`] has four coding requirements. Each is met at the reviewed head (`9dfe9a1`), per the whole-PR diff against the merge base `9d45c6d`. 1. Error paths must not return -1. The w…
Out of Scope Changes check		All changes in src/engine/lua.cc relate to `#3731`. The error-return changes, the argument-check reordering, the lua_pop removal and the typo fix are direct requirements. The new else branch logs "m.set…

Full details: Title check

Explanation

The title names the main change: Lua m.setvar() error paths returned -1, which left garbage on the Lua stack. The changeset replaces those -1 returns with zero return values in src/engine/lua.cc and adds regression cases for the error paths. The 'fix:' prefix states the change type. The title is specific and matches the linked issue and PR objectives.

Full details: Linked Issues check

Explanation

Directly linked issue [#3731] has four coding requirements. Each is met at the reviewed head (9dfe9a1), per the whole-PR diff against the merge base 9d45c6d.

Error paths must not return -1. The wrong-argument-count path (nargs != 2) and the no-dot path now return 0. A new NULL-argument path also returns 0. Lua then gives the script nil.
Argument checks must run before the std::string locals exist. The diff moves the std::string locals below the argument checks. It replaces luaL_checkstring() with lua_tostring(), so a non-string argument cannot longjmp out of the C++ frame. The NULL check is kept and now logs a debug message. A table argument returns NULL from lua_tostring(), so the test case for a table value covers this path.
The lua_pop(L, 2) that removed the value before var_value was used is removed.
The debug typo is fixed: "function must have 2 arguments".

Regression coverage: collection-lua.json adds five cases with scripts setvar-error-nodot.lua, setvar-error-onearg.lua, setvar-error-unknown-collection.lua, setvar-error-table.lua and setvar-tx.lua. Each case expects nil from m.setvar and a debug message. The TX case checks that tx.a is set to x. The issue asks for a case that checks nil. These cases cover it.

Open point: a reviewer reported Windows segfaults. The provided comments do not include a Windows retest. This affects build verification only. I did not find a code requirement in #3731 that is unmet. The result is not affected by this point.

Full details: Out of Scope Changes check

Explanation

All changes in src/engine/lua.cc relate to #3731. The error-return changes, the argument-check reordering, the lua_pop removal and the typo fix are direct requirements. The new else branch logs "m.setvar: Unknown collection". It is a debug message only, and it supports the unknown-collection regression case that the PR author and reviewer discussed. It does not change any return value or TX/IP/other collection behavior. The test files are regression fixtures for #3731 behavior. The collection-lua.json entries cover only the setvar paths. I found no unrelated source or test changes in the reviewed diff.

Finishing Touches

Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Source mode: Auto

Repository: Unavailable

Branch: Unavailable

File: No configuration file from this repository applied

Commit: Unavailable

Layers: defaults

Review profile: CHILL

Plan: Advanced

Run ID: `87dca497-caac-44c9-a1a1-611cdf934d04`

Commits

Reviewing files that changed from the base of the PR and between `3b81dca` and `9dfe9a1`.

Files selected for processing: 3

`src/engine/lua.cc`

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

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

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Copilot

🟢 Approval recommended

No active review issues remain, and the changes include targeted validation and regression coverage for the corrected invalid-call behavior.

No open issues 🎉

What changed in this PR

This pull request fixes Lua::setvar() so invalid calls return no Lua values instead of exposing stale stack data, while improving argument validation and adding regression coverage.

Changes:

Return 0 for incorrect argument counts and variable names without collections.
Validate Lua string arguments before constructing C++ strings.
Remove unsafe stack popping and correct the debug-message typo.
Add regression cases verifying invalid calls evaluate to nil.

File	Description
`test/test-cases/regression/collection-lua.json`	Adds regression cases for invalid `m.setvar()` calls.
`test/test-cases/data/setvar-error-onearg.lua`	Tests the one-argument error path.
`test/test-cases/data/setvar-error-nodot.lua`	Tests the missing-collection error path.
`src/engine/lua.cc`	Corrects `setvar()` error handling, argument validation order, stack handling, and debug text.

_{Review effort: Lite}

airween · 2026-11-01T20:25:23Z

                    t->m_rules->m_secWebAppId.m_value,
            var_value);

    }


An unknown collection, e.g. m.setvar("FOO.bar", "x"), still falls through all the branches below without a message. Could you add a final else after the USER branch (after line 404), with a level 8 message like the one above, and the name escaped as in #3730?

else { ms_dbg_a(t, 8, "m.setvar: Unknown collection: " + utils::string::toHexIfNeeded(collection) + "."); }

Done in 3b81dca: a final else logs m.setvar: Unknown collection: <name>. at level 8, with the name passed through toHexIfNeeded().

airween · 2026-11-01T20:30:55Z

@fzipi,

I also started to work on your issue #3731, and here are the test cases for that. They follow your two cases, so they can go at the end of collection-lua.json:

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

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, a successful TX set as a control:

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, after your two cases:

  {
    "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\""
    ]
  }

The first one needs the new else branch, so it fails without it. The second one passes with and without this PR, it's only there to show that the success path still works and returns nil.

Feel free to add them to this PR.

@airween

m.setvar("FOO.bar", "x") fell through every collection branch and was ignored without a message. Log it at level 8, like the other m.setvar() errors, with the name escaped. Tests: assert the debug message on each error path as well as the nil return, since m.setvar() also returns nil on success. Add cases for an unknown collection, a successful TX set (both from @airween), and a non-string value, which must fail the script with a Lua error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiUJuqXNvvpytiGRiDFyTy

fzipi · 2026-11-02T01:16:47Z

Thanks @airween. I added both of your cases as they are in 3b81dca, with the new else branch. I also:

made the two original cases check the error debug message as well as the nil return, because m.setvar() returns nil on success too, so nil alone can't tell the paths apart;
added a case for a non-string value, m.setvar("tx.a", {}). It must fail the script with Lua's bad argument #2 to 'setvar', which covers the reordering of luaL_checkstring().

collection-lua.json now has 12 cases and they all pass, and the full make check gives 5142 total, 0 fail (macOS, Lua 5.5.1).

v2's l_setvar() has the same return -1 and lua_pop() problem, so I'm opening a separate v2 PR for it.

airween · 2026-11-02T08:24:32Z

@fzipi,

thanks for your changes. Unfortunately the Windows tests were failed, all of them dropped a segfault, eg this. Please take a look at them.

luaL_checkstring() raises a Lua error, which longjmps out of the C++ Lua::setvar() frame. The MSVC build segfaults on that path. Check the arguments with lua_tostring() and return nil with a debug message, like the other m.setvar() error paths.

sonarqubecloud · 2026-11-12T09:35:24Z

Quality Gate passed

Issues
1 New issue
2 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

airween requested a lite review from Copilot November 1, 2026 20:18

Copilot started reviewing on behalf of airween November 1, 2026 20:19

View session

Copilot AI reviewed Nov 1, 2026

View reviewed changes

airween requested changes Nov 1, 2026

View reviewed changes

This was referenced Nov 2, 2026

fix: Lua m.setvar() returned -1 or a status, handing the script stack values (v2) #3736

Merged

Lua m.getvar()/m.getvars(): long transformation tables crash, and "none" doesn't reset in v3 #3737

Open

airween mentioned this pull request Nov 2, 2026

fix: Lua m.getvar()/m.getvars() transformation arguments #3738

Open

Navigation Menu

Repository navigation

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

fix: Lua m.setvar() returned -1, handing the script stack garbage#3732
fzipi wants to merge 3 commits into
owasp-modsecurity:v3/masterfrom
fzipi:fix/lua-setvar-return-3731

fzipi commented Nov 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

airween Nov 1, 2026

Uh oh!

fzipi Nov 2, 2026

Uh oh!

airween commented Nov 1, 2026

Uh oh!

fzipi commented Nov 2, 2026

Uh oh!

airween commented Nov 2, 2026

Uh oh!

sonarqubecloud Bot commented Nov 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

fzipi commented Nov 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what

why

tests

references

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

airween Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

fzipi Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

airween commented Nov 1, 2026

Uh oh!

fzipi commented Nov 2, 2026

Uh oh!

airween commented Nov 2, 2026

Uh oh!

sonarqubecloud Bot commented Nov 12, 2026

Quality Gate passed

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fzipi commented Nov 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 1, 2026 •

edited

Loading