From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7210A4696C3 for ; Thu, 16 Apr 2020 02:36:16 +0300 (MSK) From: Nikita Pettik Date: Thu, 16 Apr 2020 02:36:14 +0300 Message-Id: <3ee69eaa4ff23647e9ae6fe63e2ae77c427ffe01.1586993218.git.korablev@tarantool.org> Subject: [Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org In case accessing previous error doesn't come alongside with incrementing its reference counter, it may lead to use-after-free bug. Consider following scenario: _, err = foo() -- foo() returns diagnostic error stack preve = err.prev -- err.prev ref. counter == 1 err:set_prev(nil) -- err.prev ref. counter == 0 so err.prev is destroyed preve -- accessing already freed memory To avoid that let's increment reference counter of .prev member while calling error.prev and set corresponding gc finalizer (error_unref()). Closes #4887 --- Branch: https://github.com/tarantool/tarantool/tree/np/gh-4887-ref-error-on-prev Issue: https://github.com/tarantool/tarantool/issues/4887 I guess @ChangeLog doesn't make any sense here since current patch is a fix for feature which both come in one release (i.e. 'hotfix'). extra/exports | 1 + src/lib/core/diag.c | 20 +++++++++++++++++ src/lib/core/diag.h | 21 ++---------------- src/lua/error.lua | 5 +++++ test/box/error.result | 48 +++++++++++++++++++++++++++++++++++++++++ test/box/error.test.lua | 19 ++++++++++++++++ 6 files changed, 95 insertions(+), 19 deletions(-) diff --git a/extra/exports b/extra/exports index a9add2cc1..5b0b9e2bf 100644 --- a/extra/exports +++ b/extra/exports @@ -241,6 +241,7 @@ box_error_last box_error_clear box_error_set error_set_prev +error_unref box_latch_new box_latch_delete box_latch_lock diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c index e143db18d..787f0f82a 100644 --- a/src/lib/core/diag.c +++ b/src/lib/core/diag.c @@ -31,6 +31,26 @@ #include "diag.h" #include "fiber.h" +void +error_unref(struct error *e) +{ + assert(e->refs > 0); + struct error *to_delete = e; + while (--to_delete->refs == 0) { + /* Unlink error from lists completely.*/ + struct error *cause = to_delete->cause; + assert(to_delete->effect == NULL); + if (to_delete->cause != NULL) { + to_delete->cause->effect = NULL; + to_delete->cause = NULL; + } + to_delete->destroy(to_delete); + if (cause == NULL) + return; + to_delete = cause; + } +} + int error_set_prev(struct error *e, struct error *prev) { diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h index 7a5454d1c..beb6a7528 100644 --- a/src/lib/core/diag.h +++ b/src/lib/core/diag.h @@ -118,25 +118,8 @@ error_ref(struct error *e) e->refs++; } -static inline void -error_unref(struct error *e) -{ - assert(e->refs > 0); - struct error *to_delete = e; - while (--to_delete->refs == 0) { - /* Unlink error from lists completely.*/ - struct error *cause = to_delete->cause; - assert(to_delete->effect == NULL); - if (to_delete->cause != NULL) { - to_delete->cause->effect = NULL; - to_delete->cause = NULL; - } - to_delete->destroy(to_delete); - if (cause == NULL) - return; - to_delete = cause; - } -} +void +error_unref(struct error *e); /** * Unlink error from its effect. For instance: diff --git a/src/lua/error.lua b/src/lua/error.lua index bdc9c714d..e6a1c3abe 100644 --- a/src/lua/error.lua +++ b/src/lua/error.lua @@ -35,6 +35,9 @@ exception_get_int(struct error *e, const struct method_info *method); int error_set_prev(struct error *e, struct error *prev); + +void +error_unref(struct error *e); ]] local REFLECTION_CACHE = {} @@ -103,6 +106,8 @@ end local function error_prev(err) local e = err._cause; if e ~= nil then + e._refs = e._refs + 1 + e = ffi.gc(e, ffi.C.error_unref) return e else return nil diff --git a/test/box/error.result b/test/box/error.result index df6d0ebc0..49e35b942 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -831,3 +831,51 @@ assert(box.error.last() == e1) | --- | - true | ... + +-- gh-4887: accessing 'prev' member also refs it so that after +-- error is gone, its 'prev' is staying alive. +-- +lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] + | --- + | ... +box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true}) + | --- + | ... +s = box.schema.space.create('withdata') + | --- + | ... +pk = s:create_index('pk') + | --- + | ... +idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}}) + | --- + | ... + +function test_func() return pcall(s.insert, s, {1}) end + | --- + | ... +ok, err = test_func() + | --- + | ... +preve = err.prev + | --- + | ... +err:set_prev(nil) + | --- + | ... +err.prev + | --- + | - null + | ... +preve + | --- + | - '[string "return function(tuple) local json = require(''..."]:1: attempt to call + | global ''require'' (a nil value)' + | ... + +s:drop() + | --- + | ... +box.schema.func.drop('runtimeerror') + | --- + | ... diff --git a/test/box/error.test.lua b/test/box/error.test.lua index 41baed52d..dbc4d143f 100644 --- a/test/box/error.test.lua +++ b/test/box/error.test.lua @@ -229,3 +229,22 @@ box.error({code = 111, reason = "err"}) box.error.last() box.error(e1) assert(box.error.last() == e1) + +-- gh-4887: accessing 'prev' member also refs it so that after +-- error is gone, its 'prev' is staying alive. +-- +lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] +box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true}) +s = box.schema.space.create('withdata') +pk = s:create_index('pk') +idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}}) + +function test_func() return pcall(s.insert, s, {1}) end +ok, err = test_func() +preve = err.prev +err:set_prev(nil) +err.prev +preve + +s:drop() +box.schema.func.drop('runtimeerror') -- 2.17.1