[Tarantool-patches] [PATCH] box/error: ref error.prev while accessing it
Nikita Pettik
korablev at tarantool.org
Thu Apr 16 02:36:14 MSK 2020
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
More information about the Tarantool-patches
mailing list