[Tarantool-patches] [PATCH v2 2/2] box/error: ref error.prev while accessing it

[Nikita Pettik]

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