[8.0] Fix lua-enable-insecure-api default value cannot be changed to yes (#3548) - #4188
Conversation
…alkey-io#3548) The default value of lua-enable-insecure-api cannot be safely changed from no to yes due to two issues: 1. In createEngineContext(), lua_enable_insecure_api was hardcoded to 0 before initializing Lua states, so deprecated APIs (newproxy, setfenv, getfenv) were never registered in the global table regardless of the actual config value. Once the global table is locked, the config change has no effect. 2. lua_insecure_api_current was initialized to 0 (struct zero-init) and never synced with the final config value. If the default was changed to yes(1), a subsequent CONFIG SET no would see both values as 0 and skip the evalReset() call in updateLuaEnableInsecureApi(). Fix by reading the real config via isLuaInsecureAPIEnabled() in createEngineContext() before Lua state initialization, and syncing lua_insecure_api_current after all config sources (default, config file, command-line args) are applied. 8.0 backport note: only fix 2 (the lua_insecure_api_current sync in server.c) applies to this branch. Bug 1 does not exist on 8.0: there is no per-engine-context lua_enable_insecure_api field hardcoded to 0 -- the Lua engine has not been modularized here (Lua scripting lives in src/script_lua.c / src/eval.c; there is no engine_lua.c). The deprecated-API allowlist gate reads server.lua_enable_insecure_api directly (src/script_lua.c luaNewIndexAllowList), and scriptingInit() runs in initServer() after loadServerConfig(), so the config-file value is already honored at Lua state initialization. The engine_lua.c hunk is therefore dropped. Validated: the two new tests fail on 8.0 without the server.c sync and pass with it. Signed-off-by: Binbin <binloveplay1314@qq.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one correctness gap in the backport: it fixes the eval Lua state, but the separate functions Lua engine still keeps the startup insecure-API allowlist across CONFIG SET changes. The new tests only exercise EVAL, so that stale path remains unverified.
3 participants
Backport of #3548 to 8.0. Same adaptation as the 9.0 (#4182) and 8.1 (#4183) backports.
Adaptation for 8.0: only the second fix from the original PR (the
lua_insecure_api_currentsync in server.c) applies here. The first fix doesnot exist on 8.0: the Lua engine is not modularized on this branch (Lua
scripting lives in src/script_lua.c / src/eval.c; there is no engine_lua.c).
The deprecated-API allowlist gate reads
server.lua_enable_insecure_apidirectly in
luaNewIndexAllowList(src/script_lua.c), andscriptingInit()runs in
initServer()afterloadServerConfig(), so the config-file value isalready honored at Lua state initialization. The engine_lua.c hunk is
therefore dropped. Note: on this branch
updateLuaEnableInsecureApi()callsscriptingReset()rather thanevalReset()— behavior is equivalent forthis fix.
Validation on 8.0:
unit/scriptingsuite passes with the fixgetfenv()remains accessible after
CONFIG SET lua-enable-insecure-api no