From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 DB9BC4696C3 for ; Fri, 17 Apr 2020 23:16:48 +0300 (MSK) From: Nikita Pettik Date: Fri, 17 Apr 2020 23:16:46 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH v2 2/2] 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 --- extra/exports | 1 + src/lib/core/diag.c | 20 ++++++++++++ src/lib/core/diag.h | 21 ++----------- src/lua/error.lua | 9 ++++++ test/box/error.result | 68 +++++++++++++++++++++++++++++++++++++++++ test/box/error.test.lua | 26 ++++++++++++++++ 6 files changed, 126 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 5ddf543cb..90c4fa21a 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 fe852e640..ae49fd8b0 100644 --- a/src/lib/core/diag.h +++ b/src/lib/core/diag.h @@ -122,25 +122,8 @@ error_ref(struct error *e) return 0; } -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 7a2435b0a..9f7a90c03 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,12 @@ end local function error_prev(err) local e = err._cause; if e ~= nil then + local INT32_MAX = 2147483647 + if e._refs >= INT32_MAX then + error("Too many references to error object") + end + 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..ee9dc2a85 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -831,3 +831,71 @@ 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 + | --- + | ... +gc_err = setmetatable({preve}, {__mode = 'v'}) + | --- + | ... +err:set_prev(nil) + | --- + | ... +err.prev + | --- + | - null + | ... +collectgarbage('collect') + | --- + | - 0 + | ... +-- Still one reference to err.prev so it should not be collected. +-- +gc_err + | --- + | - - '[string "return function(tuple) local json = require(''..."]:1: attempt to call + | global ''require'' (a nil value)' + | ... +preve = nil + | --- + | ... +collectgarbage('collect') + | --- + | - 0 + | ... +gc_err + | --- + | - [] + | ... + +s:drop() + | --- + | ... +box.schema.func.drop('runtimeerror') + | --- + | ... diff --git a/test/box/error.test.lua b/test/box/error.test.lua index 41baed52d..3ae197379 100644 --- a/test/box/error.test.lua +++ b/test/box/error.test.lua @@ -229,3 +229,29 @@ 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 +gc_err = setmetatable({preve}, {__mode = 'v'}) +err:set_prev(nil) +err.prev +collectgarbage('collect') +-- Still one reference to err.prev so it should not be collected. +-- +gc_err +preve = nil +collectgarbage('collect') +gc_err + +s:drop() +box.schema.func.drop('runtimeerror') -- 2.17.1