gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

Fix #11030, use lua_rawget to avoid triggering metatables and crash. (#11032)

Browse Source 
Fix #11030, use lua_rawget to avoid triggering metatables.

#11030 shows how return `_G` from the Lua script (either function or eval), cause the
Lua interpreter to Panic and the Redis processes to exit with error code 1.
Though return `_G` only panic on Redis 7 and 6.2.7, the underline issue exists on older
versions as well (6.0 and 6.2). The underline issue is returning a table with a metatable
such that the metatable raises an error.

The following example demonstrate the issue:
```
127.0.0.1:6379> eval "local a = {}; setmetatable(a,{__index=function() foo() end}) return a" 0
Error: Server closed the connection
```
```
PANIC: unprotected error in call to Lua API (user_script:1: Script attempted to access nonexistent global variable 'foo')
```

The Lua panic happened because when returning the result to the client, Redis needs to
introspect the returning table and transform the table into a resp. In order to scan the table,
Redis uses `lua_gettable` api which might trigger the metatable (if exists) and might raise an error.
This code is not running inside `pcall` (Lua protected call), so raising an error causes the
Lua to panic and exit. Notice that this is not a crash, its a Lua panic that exit with error code 1.

Returning `_G` panics on Redis 7 and 6.2.7 because on those versions `_G` has a metatable
that raises error when trying to fetch a none existing key.

### Solution

Instead of using `lua_gettable` that might raise error and cause the issue, use `lua_rawget`
that simply return the value from the table without triggering any metatable logic.
This is promised not to raise and error.

The downside of this solution is that it might be considered as breaking change, if someone
rely on metatable in the returned value. An alternative solution is to wrap this entire logic
with `pcall` (Lua protected call), this alternative require a much bigger refactoring.

### Back Porting

The same fix will work on older versions as well (6.2, 6.0). Notice that on those version,
the issue can cause Redis to crash if inside the metatable logic there is an attempt to accesses
Redis (`redis.call`). On 7.0, there is not crash and the `redis.call` is executed as if it was done
from inside the script itself.

### Tests

Tests was added the verify the fix
This commit is contained in:
Meir Shpilraien (Spielrein) 2022-07-26 10:33:50 +03:00 committed by GitHub
parent 5032de50f2
commit 020e046b42
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 24 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/script_lua.c
View File

@ -628,7 +628,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle error reply. */
/* we took care of the stack size on function start */
lua_pushstring(lua,"err");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TSTRING) {
lua_pop(lua, 1); /* pop the error message, we will use luaExtractErrorInformation to get error information */
@ -646,7 +646,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle status reply. */
lua_pushstring(lua,"ok");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TSTRING) {
sds ok = sdsnew(lua_tostring(lua,-1));
@ -660,7 +660,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle double reply. */
lua_pushstring(lua,"double");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TNUMBER) {
addReplyDouble(c,lua_tonumber(lua,-1));
@ -671,7 +671,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle big number reply. */
lua_pushstring(lua,"big_number");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TSTRING) {
sds big_num = sdsnewlen(lua_tostring(lua,-1), lua_strlen(lua,-1));
@ -685,16 +685,16 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle verbatim reply. */
lua_pushstring(lua,"verbatim_string");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TTABLE) {
lua_pushstring(lua,"format");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TSTRING){
char* format = (char*)lua_tostring(lua,-1);
lua_pushstring(lua,"string");
lua_gettable(lua,-3);
lua_rawget(lua,-3);
t = lua_type(lua,-1);
if (t == LUA_TSTRING){
size_t len;
@ -711,7 +711,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle map reply. */
lua_pushstring(lua,"map");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TTABLE) {
int maplen = 0;
@ -734,7 +734,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
/* Handle set reply. */
lua_pushstring(lua,"set");
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TTABLE) {
int setlen = 0;
@ -761,7 +761,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu
while(1) {
/* we took care of the stack size on function start */
lua_pushnumber(lua,j++);
lua_gettable(lua,-2);
lua_rawget(lua,-2);
t = lua_type(lua,-1);
if (t == LUA_TNIL) {
lua_pop(lua,1);

14
tests/unit/scripting.tcl
View File

@ -59,6 +59,20 @@ start_server {tags {"scripting"}} {
run_script {return 'hello'} 0
} {hello}
test {EVAL - Return _G} {
run_script {return _G} 0
} {}
test {EVAL - Return table with a metatable that raise error} {
run_script {local a = {}; setmetatable(a,{__index=function() foo() end}) return a} 0
} {}
test {EVAL - Return table with a metatable that call redis} {
run_script {local a = {}; setmetatable(a,{__index=function() redis.call('set', 'x', '1') end}) return a} 0
# make sure x was not set
r get x
} {}
test {EVAL - Lua integer -> Redis protocol type conversion} {
run_script {return 100.5} 0
} {100}