[7.2] Fix lua-enable-insecure-api default value cannot be changed to yes (#3548) - #4199
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. 7.2 backport note: only fix 2 (the lua_insecure_api_current sync in server.c) applies to this branch. Bug 1 does not exist on 7.2: 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 7.2 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}
| * config sources (default, config file, command-line args) have been | ||
| * applied, so that updateLuaEnableInsecureApi() can correctly detect | ||
| * subsequent changes via CONFIG SET. */ | ||
| server.lua_insecure_api_current = server.lua_enable_insecure_api; |
There was a problem hiding this comment.
lua_insecure_api_current needs to be initialized before initServer()/scriptingInit(1), not after moduleLoadFromQueue(). scriptingInit(1) already ran in src/server.c:2740, and startup modules are allowed to execute commands from OnLoad (src/module.c:12179-12181; tests/modules/basics.c:948-953). If the server starts with lua-enable-insecure-api yes and a loadmodule’s OnLoad does CONFIG SET lua-enable-insecure-api no (possible when enable-protected-configs yes), updateLuaEnableInsecureApi() sees _current == _enable_insecure_api == 0 and skips scriptingReset() (src/config.c:2580-2585), so the deprecated APIs stay enabled while this line records the config as no. Initialize _current right after loadServerConfig() (src/server.c:7265) and before initServer() (src/server.c:7316) so later startup-time CONFIG SETs are compared against the real startup value.
3 participants
Backport of #3548 to 7.2. Same adaptation as the 9.0 (#4182), 8.1 (#4183),
and 8.0 (#4188) backports.
Adaptation for 7.2: only the second fix from the original PR (the
lua_insecure_api_currentsync in server.c) applies here. The first fix doesnot exist on 7.2: 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 7.2:
unit/scriptingsuite passes with the fixgetfenv()remains accessible after
CONFIG SET lua-enable-insecure-api no