summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMeir Shpilraien (Spielrein) <meir@redis.com>2022-07-26 10:33:50 +0300
committerOran Agra <oran@redislabs.com>2022-09-21 22:42:01 +0300
commitddf1bcf788c81b6b89d4c2949a437633fea09f91 (patch)
tree750969554962cd4c72af0230701121d82db1df86
parent5da7fdb78532e3223cf1d91bb3e935718beccce8 (diff)
downloadredis-ddf1bcf788c81b6b89d4c2949a437633fea09f91.tar.gz
Fix #11030, use lua_rawget to avoid triggering metatables and crash. (#11032)
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 (cherry picked from commit 020e046b4210f64a614d7ec51b4ee09f746e5350)
-rw-r--r--src/script_lua.c20
-rw-r--r--tests/unit/scripting.tcl14
2 files changed, 24 insertions, 10 deletions
diff --git a/src/script_lua.c b/src/script_lua.c
index 22eb58102..29edf344b 100644
--- a/src/script_lua.c
+++ b/src/script_lua.c
@@ -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);
diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl
index f1c05a4d4..cca24c1bd 100644
--- a/tests/unit/scripting.tcl
+++ b/tests/unit/scripting.tcl
@@ -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}