[8.1] Fix lua-enable-insecure-api default value cannot be changed to yes (#3548)#4183
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.1 backport note: only fix valkey-io#2 (the lua_insecure_api_current sync in server.c) applies to this branch. Bug valkey-io#1 does not exist on 8.1: there is no per-engine-context lua_enable_insecure_api field hardcoded to 0 -- the Lua engine has not been modularized here (src/lua/, not src/modules/lua/). The deprecated-API allowlist gate reads server.lua_enable_insecure_api directly (script_lua.c luaNewIndexAllowList), and evalInit() 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.1 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.
moduleLoadFromQueue() has already run before this line (src/server.c:7167-7170, src/module.c:12189-12205), and startup modules are allowed to issue ValkeyModule_Call() from ValkeyModule_OnLoad() (tests/modules/basics.c:948-953). That means a module loaded from the config file can call CONFIG SET lua-enable-insecure-api no during OnLoad().
If the server booted with lua-enable-insecure-api yes and enable-protected-configs yes, updateLuaEnableInsecureApi() will compare the zero-initialized server.lua_insecure_api_current against the new 0 and skip evalReset() (src/config.c:2612-2618), so the EVAL Lua state stays initialized with the old yes setting for the rest of startup. Move this sync above moduleLoadFromQueue() so the first CONFIG SET during OnLoad() is compared against the actual startup value.
There was a problem hiding this comment.
@dubey02 looks like a concern worth checking, would you please check! Thanks!
There was a problem hiding this comment.
Thanks @sarthakaggarwal97, I checked that the concern is valid: moduleLoadFromQueue() runs before this sync, so a startup module calling CONFIG SET lua-enable-insecure-api no in OnLoad() would skip the reset. But this ordering is inherited from the original #3548 as merged on unstable, so it affects unstable/9.1 and all other backports equally. I think we should keep this backport same as merged commit and fix the ordering on unstable first, then backport it to the release branches separately.
There was a problem hiding this comment.
yeah I agree with you, I kind of came towards the same conclusion hence merged it. If it's worth fixing, we should raise a PR to unstable as well.
3 participants
Backport of #3548 to 8.1. Same adaptation as the 9.0 backport in #4182.
Adaptation for 8.1: 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.1: the Lua engine is not modularized on this branch (src/lua/,
not src/modules/lua/), there is no per-engine-context
lua_enable_insecure_apifield hardcoded to 0, and the deprecated-APIallowlist gate reads
server.lua_enable_insecure_apidirectly inluaNewIndexAllowList(script_lua.c). SinceevalInit()runs ininitServer()afterloadServerConfig(), the config-file value is alreadyhonored at Lua state initialization. The engine_lua.c hunk is therefore
dropped.
Validation on 8.1:
unit/scriptingsuite passes with the fixgetfenv()remains accessible after
CONFIG SET lua-enable-insecure-api nobecauseupdateLuaEnableInsecureApi()sees0 != 0as false and skipsevalReset()