Update unstable with CVE-2025-49844, CVE-2025-46818, CVE-2025-46819, CVE-2025-46817#13
Conversation
WalkthroughThis change enhances Lua lexical analysis, parser token handling, and script security by refactoring delimiter sequence parsing to use size_t types, improving bounds checking in table access, adding a configuration flag for deprecated API support, and introducing read-only protection for basic Lua type metatables during initialization with a deprecated API gating mechanism. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@deps/lua/src/llex.c`:
- Around line 210-226: The return logic in skip_sep is inverted causing a plain
'[' to return 0 instead of 1; update the final return to return (ls->current ==
s) ? count + 2 : (count == 0) ? 1 : 0 so that a lone bracket (no '=') yields 1
and unfinished '[=...' yields 0; modify the return expression in skip_sep
(function skip_sep, using LexState *ls and ls->current) accordingly.
In `@src/eval.c`:
- Around line 255-257: The current luaSetTableProtectionForBasicTypes
implementation retrieves metatables with lua_getmetatable() but never enables
read-only mode; modify luaSetTableProtectionForBasicTypes to call
lua_enablereadonlytable(lua, -1, 1) immediately after each successful
lua_getmetatable() and before removing the metatable from the stack (e.g.,
before any lua_pop), so each basic-type metatable is actually marked read-only;
ensure you do this for every metatable retrieval in that function.
In `@src/script_lua.c`:
- Around line 1385-1413: The function luaSetTableProtectionForBasicTypes
currently fetches metatables for basic types but never applies protection;
update luaSetTableProtectionForBasicTypes so that after lua_getmetatable(lua,
-1) returns true you call luaSetTableProtectionRecursively(lua) to mark the
metatable readonly before popping it, preserving the existing stack pops (pop
metatable then pop dummy value) and keeping the rest of the loop unchanged.
🧹 Nitpick comments (1)
tests/unit/scripting.tcl (1)
1193-1212: Loosen the nil-index error match to avoid brittle failures.Error formatting can vary across execution paths; matching the core phrase is usually sufficient.
Proposed tweak
- [string match "*attempt to index a nil value script*" $e] || + [string match "*attempt to index a nil value*" $e] ||
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deps/lua/src/llex.cdeps/lua/src/lparser.cdeps/lua/src/ltable.csrc/config.csrc/eval.csrc/function_lua.csrc/script_lua.csrc/script_lua.hsrc/server.htests/unit/scripting.tcl
🧰 Additional context used
🧬 Code graph analysis (4)
src/eval.c (1)
src/script_lua.c (1)
luaSetTableProtectionForBasicTypes(1386-1413)
deps/lua/src/lparser.c (1)
deps/lua/src/llex.c (2)
luaX_setinput(140-152)luaX_next(456-464)
src/script_lua.h (1)
src/script_lua.c (1)
luaSetTableProtectionForBasicTypes(1386-1413)
src/function_lua.c (1)
src/script_lua.c (1)
luaSetTableProtectionForBasicTypes(1386-1413)
🔇 Additional comments (14)
deps/lua/src/ltable.c (1)
436-438: Clearer bounds check; avoids wraparound reliance.src/script_lua.h (1)
54-54: LGTM: public API exposed cleanly.Declaration aligns with the new helper usage and keeps the header consistent.
src/server.h (1)
2334-2335: LGTM: scripting config flag addition looks consistent.Placement next to other Lua scripting config fields is appropriate.
src/function_lua.c (1)
498-499: LGTM: protection hook added at engine init.The placement aligns with the globals/metatable setup sequence.
deps/lua/src/lparser.c (1)
383-397: Stack anchoring for the source name looks good.Line 387: keeping
tnameon the stack during parsing and popping it afterclose_funcbalances the stack and avoids premature GC of the source string.src/config.c (1)
3126-3130: LGTM: new Lua deprecated-API flag registration.Line 3129: the hidden, immutable config entry is consistent with a startup-only compatibility toggle.
src/script_lua.c (2)
80-85: Clear separation of deprecated builtins.Line 80: isolating deprecated globals into a dedicated list improves clarity for gated exposure.
1297-1338: Allow-list gating for deprecated APIs looks good.Line 1310: the explicit
allowed/deprecated check cleanly ties exposure tolua_enable_deprecated_api.deps/lua/src/llex.c (3)
140-151: Token reset on input setup is fine.Line 141: clearing
ls->t.tokenavoids stale token state when reusingLexState.
230-282: Long-string slicing update looks consistent.Line 279: using
sepand2 * sepmatches the new delimiter length semantics.
355-377:sep >= 2gating inllexaligns with the new contract.Line 356: the updated checks are correct assuming
skip_sepreturns1for plain[and0for unfinished[=....tests/unit/scripting.tcl (3)
358-395: Edge-caseunpackcoverage looks solid.These cases exercise invalid index ranges and nil padding well against the new bounds checks.
1242-1286: Large-memory coverage looks good.The scenarios target the intended pathological sizes and parser edge cases.
1288-1335: Deprecated API gating tests are clear and complete.The enabled/disabled branches validate the config flag behavior cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /* | ||
| ** reads a sequence '[=*[' or ']=*]', leaving the last bracket. | ||
| ** If a sequence is well-formed, return its number of '='s + 2; otherwise, | ||
| ** return 1 if there is no '='s or 0 otherwise (an unfinished '[==...'). | ||
| */ | ||
| static size_t skip_sep (LexState *ls) { | ||
| size_t count = 0; | ||
| int s = ls->current; | ||
| lua_assert(s == '[' || s == ']'); | ||
| save_and_next(ls); | ||
| while (ls->current == '=') { | ||
| save_and_next(ls); | ||
| count++; | ||
| } | ||
| return (ls->current == s) ? count : (-count) - 1; | ||
| return (ls->current == s) ? count + 2 | ||
| : (count == 0) ? 0 | ||
| : 1; |
There was a problem hiding this comment.
skip_sep return values appear inverted, which breaks plain [ tokens.
Line 224-226: with the current return values, a normal [ (no = and no second bracket) returns 0, which llex treats as an invalid long-string delimiter and throws. The contract (and llex) expects 1 for “no =” and 0 for unfinished [=....
🐛 Proposed fix
- return (ls->current == s) ? count + 2
- : (count == 0) ? 0
- : 1;
+ return (ls->current == s) ? count + 2
+ : (count == 0) ? 1
+ : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* | |
| ** reads a sequence '[=*[' or ']=*]', leaving the last bracket. | |
| ** If a sequence is well-formed, return its number of '='s + 2; otherwise, | |
| ** return 1 if there is no '='s or 0 otherwise (an unfinished '[==...'). | |
| */ | |
| static size_t skip_sep (LexState *ls) { | |
| size_t count = 0; | |
| int s = ls->current; | |
| lua_assert(s == '[' || s == ']'); | |
| save_and_next(ls); | |
| while (ls->current == '=') { | |
| save_and_next(ls); | |
| count++; | |
| } | |
| return (ls->current == s) ? count : (-count) - 1; | |
| return (ls->current == s) ? count + 2 | |
| : (count == 0) ? 0 | |
| : 1; | |
| static size_t skip_sep (LexState *ls) { | |
| size_t count = 0; | |
| int s = ls->current; | |
| lua_assert(s == '[' || s == ']'); | |
| save_and_next(ls); | |
| while (ls->current == '=') { | |
| save_and_next(ls); | |
| count++; | |
| } | |
| return (ls->current == s) ? count + 2 | |
| : (count == 0) ? 1 | |
| : 0; | |
| } |
🤖 Prompt for AI Agents
In `@deps/lua/src/llex.c` around lines 210 - 226, The return logic in skip_sep is
inverted causing a plain '[' to return 0 instead of 1; update the final return
to return (ls->current == s) ? count + 2 : (count == 0) ? 1 : 0 so that a lone
bracket (no '=') yields 1 and unfinished '[=...' yields 0; modify the return
expression in skip_sep (function skip_sep, using LexState *ls and ls->current)
accordingly.
| /* Set metatables of basic types (string, number, nil etc.) readonly. */ | ||
| luaSetTableProtectionForBasicTypes(lua); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "luaSetTableProtectionForBasicTypes" src/script_lua.c -A 30Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1353
Ensure basic‑type metatables are actually set read‑only.
The luaSetTableProtectionForBasicTypes function (lines 1386-1413 in src/script_lua.c) retrieves metatables via lua_getmetatable() but immediately pops them without applying any protection. Add lua_enablereadonlytable(lua, -1, 1) before popping each metatable to actually enforce read-only status, otherwise the metatables remain mutable and the hardening is ineffective.
Suggested fix in src/script_lua.c
void luaSetTableProtectionForBasicTypes(lua_State *lua) {
static const int types[] = {
LUA_TSTRING,
LUA_TNUMBER,
LUA_TBOOLEAN,
LUA_TNIL,
LUA_TFUNCTION,
LUA_TTHREAD,
LUA_TLIGHTUSERDATA
};
for (size_t i = 0; i < sizeof(types) / sizeof(types[0]); i++) {
/* Push a dummy value of the type to get its metatable */
switch (types[i]) {
case LUA_TSTRING: lua_pushstring(lua, ""); break;
case LUA_TNUMBER: lua_pushnumber(lua, 0); break;
case LUA_TBOOLEAN: lua_pushboolean(lua, 0); break;
case LUA_TNIL: lua_pushnil(lua); break;
case LUA_TFUNCTION: lua_pushcfunction(lua, NULL); break;
case LUA_TTHREAD: lua_newthread(lua); break;
case LUA_TLIGHTUSERDATA: lua_pushlightuserdata(lua, (void*)lua); break;
}
if (lua_getmetatable(lua, -1)) {
+ lua_enablereadonlytable(lua, -1, 1);
lua_pop(lua, 1); /* pop metatable */
}
lua_pop(lua, 1); /* pop dummy value */
}
}🤖 Prompt for AI Agents
In `@src/eval.c` around lines 255 - 257, The current
luaSetTableProtectionForBasicTypes implementation retrieves metatables with
lua_getmetatable() but never enables read-only mode; modify
luaSetTableProtectionForBasicTypes to call lua_enablereadonlytable(lua, -1, 1)
immediately after each successful lua_getmetatable() and before removing the
metatable from the stack (e.g., before any lua_pop), so each basic-type
metatable is actually marked read-only; ensure you do this for every metatable
retrieval in that function.
| /* Set the readonly flag on the metatable of basic types (string, nil etc.) */ | ||
| void luaSetTableProtectionForBasicTypes(lua_State *lua) { | ||
| static const int types[] = { | ||
| LUA_TSTRING, | ||
| LUA_TNUMBER, | ||
| LUA_TBOOLEAN, | ||
| LUA_TNIL, | ||
| LUA_TFUNCTION, | ||
| LUA_TTHREAD, | ||
| LUA_TLIGHTUSERDATA | ||
| }; | ||
|
|
||
| for (size_t i = 0; i < sizeof(types) / sizeof(types[0]); i++) { | ||
| /* Push a dummy value of the type to get its metatable */ | ||
| switch (types[i]) { | ||
| case LUA_TSTRING: lua_pushstring(lua, ""); break; | ||
| case LUA_TNUMBER: lua_pushnumber(lua, 0); break; | ||
| case LUA_TBOOLEAN: lua_pushboolean(lua, 0); break; | ||
| case LUA_TNIL: lua_pushnil(lua); break; | ||
| case LUA_TFUNCTION: lua_pushcfunction(lua, NULL); break; | ||
| case LUA_TTHREAD: lua_newthread(lua); break; | ||
| case LUA_TLIGHTUSERDATA: lua_pushlightuserdata(lua, (void*)lua); break; | ||
| } | ||
| if (lua_getmetatable(lua, -1)) { | ||
| lua_pop(lua, 1); /* pop metatable */ | ||
| } | ||
| lua_pop(lua, 1); /* pop dummy value */ | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check the function
wc -l src/script_lua.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 105
🏁 Script executed:
# Check the specific lines mentioned in the review
sed -n '1385,1413p' src/script_lua.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1189
🏁 Script executed:
# Search for luaSetTableProtectionRecursively to understand what it does
rg -n "luaSetTableProtectionRecursively" src/script_lua.c -A 15Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1422
🏁 Script executed:
# Also search for where luaSetTableProtectionForBasicTypes is called
rg -n "luaSetTableProtectionForBasicTypes" src/script_lua.c -B 2 -A 2Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 300
🏁 Script executed:
# Check if there are other functions that might protect basic type metatables
rg -n "lua_enablereadonlytable|lua_setreadonly" src/script_lua.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 385
🏁 Script executed:
# Check the context around where luaSetTableProtectionForBasicTypes is called
rg -n "luaSetTableProtectionForBasicTypes" src/ -B 5 -A 5Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2728
luaSetTableProtectionForBasicTypes doesn't actually protect metatables.
The function retrieves metatables for basic types but immediately pops them without applying any protection. Expected to make these metatables read-only (per function comment and calling code), but currently leaves them mutable. Should call luaSetTableProtectionRecursively(lua) before popping the metatable.
🔧 Proposed fix
- if (lua_getmetatable(lua, -1)) {
- lua_pop(lua, 1); /* pop metatable */
- }
+ if (lua_getmetatable(lua, -1)) {
+ luaSetTableProtectionRecursively(lua);
+ lua_pop(lua, 1); /* pop metatable */
+ }🤖 Prompt for AI Agents
In `@src/script_lua.c` around lines 1385 - 1413, The function
luaSetTableProtectionForBasicTypes currently fetches metatables for basic types
but never applies protection; update luaSetTableProtectionForBasicTypes so that
after lua_getmetatable(lua, -1) returns true you call
luaSetTableProtectionRecursively(lua) to mark the metatable readonly before
popping it, preserving the existing stack pops (pop metatable then pop dummy
value) and keeping the rest of the loop unchanged.
3 participants
Benchmark PR from qodo-benchmark#30
Summary by CodeRabbit
Release Notes
New Features
lua-enable-deprecated-apiconfiguration option to control support for deprecated Lua APIs in scripts.Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.