[Tarantool-patches] [PATCH v2 0/2] Stacked diagnostic area follow-ups

[Tarantool-patches] [PATCH v2 0/2] Stacked diagnostic area follow-ups

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-4887-ref-error-on-prev
Issue: https://github.com/tarantool/tarantool/issues/4887

Changes in v2:

 - modified test so that now it uses weak references to check that
gc collected error objects (i.e. there's no memory leaks);
 - added overflow check in error_ref() so that after 2^32 calls
of box.error.last() or error:prev() error object won't contain
broken reference counter.

Nikita Pettik (2):
  box/error: don't allow overflow of error ref counter
  box/error: ref error.prev while accessing it

 extra/exports           |  1 +
 src/lib/core/diag.c     | 21 +++++++++++++
 src/lib/core/diag.h     | 29 ++++++------------
 src/lua/error.c         |  3 +-
 src/lua/error.lua       | 13 ++++++++
 test/box/error.result   | 68 +++++++++++++++++++++++++++++++++++++++++
 test/box/error.test.lua | 26 ++++++++++++++++
 7 files changed, 140 insertions(+), 21 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v2 1/2] box/error: don't allow overflow of error ref counter

[Nikita Pettik]

There's no overflow check while incrementing error's reference counter
in error_ref(). Meanwhile, stubborn users still may achieve overflow:
each call of box.error.last() increments reference counter of error
residing in diagnostic area. As a result, 2^32 calls of box.error.last()
in a row will lead to counter overflow ergo - to unpredictable results.
Let's fix it and introduce dummy check in error_ref().
---
 src/lib/core/diag.c | 1 +
 src/lib/core/diag.h | 8 +++++++-
 src/lua/error.c     | 3 ++-
 src/lua/error.lua   | 4 ++++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index e143db18d..5ddf543cb 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -60,6 +60,7 @@ error_set_prev(struct error *e, struct error *prev)
 			 */
 			error_unlink_effect(prev);
 		}
+		assert(prev->refs < INT32_MAX);
 		error_ref(prev);
 		prev->effect = e;
 	}
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 7a5454d1c..fe852e640 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -112,10 +112,14 @@ struct error {
 	struct error *effect;
 };
 
-static inline void
+static inline int
 error_ref(struct error *e)
 {
+	assert(e->refs >= 0);
+	if (e->refs >= INT32_MAX)
+		return -1;
 	e->refs++;
+	return 0;
 }
 
 static inline void
@@ -247,6 +251,7 @@ static inline void
 diag_set_error(struct diag *diag, struct error *e)
 {
 	assert(e != NULL);
+	assert(e->refs < INT32_MAX);
 	error_ref(e);
 	diag_clear(diag);
 	error_unlink_effect(e);
@@ -271,6 +276,7 @@ diag_add_error(struct diag *diag, struct error *e)
 	 */
 	assert(e->effect == NULL);
 	assert(diag->last->effect == NULL);
+	assert(e->refs < INT32_MAX);
 	error_ref(e);
 	e->cause = diag->last;
 	diag->last->effect = e;
diff --git a/src/lua/error.c b/src/lua/error.c
index 18a990a88..5c4af79e7 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -84,7 +84,8 @@ luaT_pusherror(struct lua_State *L, struct error *e)
 	 * It also important to reference the error first and only
 	 * then set the finalizer.
 	 */
-	error_ref(e);
+	if (error_ref(e) != 0)
+		luaL_error(L, "Too many references to error object");
 	assert(CTID_CONST_STRUCT_ERROR_REF != 0);
 	struct error **ptr = (struct error **)
 		luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
diff --git a/src/lua/error.lua b/src/lua/error.lua
index bdc9c714d..7a2435b0a 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -118,6 +118,10 @@ local function error_set_prev(err, prev)
     if not ffi.istype('struct error', prev) and prev ~= nil then
         error("Usage: error1:set_prev(error2)")
     end
+    local INT32_MAX = 2147483647
+    if err._refs >= INT32_MAX then
+        error("Too many references to error object")
+    end
     local ok = ffi.C.error_set_prev(err, prev);
     if ok ~= 0 then
         error("Cycles are not allowed")
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2 1/2] box/error: don't allow overflow of error ref counter

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

Maybe just make the ref counter uint64_t? It will never
overflow then, and the patch would become of 2 lines.

Re: [Tarantool-patches] [PATCH v2 1/2] box/error: don't allow overflow of error ref counter

[Nikita Pettik]

On 18 Apr 01:54, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Maybe just make the ref counter uint64_t? It will never
> overflow then, and the patch would become of 2 lines.

"Explicit is better than implicit", so I'd prefer explicit error
raising rather than silent reducing counter modulo 2^64.
What is more, making counter unsigned still don't resolve all
problems: reset would result in premature error destruction
meanwhile some Lua object still accessing it.

[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

Re: [Tarantool-patches] [PATCH v2 0/2] Stacked diagnostic area follow-ups

[Kirill Yukhin]

Hello,

On 17 апр 23:16, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-4887-ref-error-on-prev
> Issue: https://github.com/tarantool/tarantool/issues/4887
> 
> Changes in v2:
> 
>  - modified test so that now it uses weak references to check that
> gc collected error objects (i.e. there's no memory leaks);
>  - added overflow check in error_ref() so that after 2^32 calls
> of box.error.last() or error:prev() error object won't contain
> broken reference counter.
> 
> Nikita Pettik (2):
>   box/error: don't allow overflow of error ref counter
>   box/error: ref error.prev while accessing it

I've checked your patchset into master.

However, looks like it'd be better to just replace int32 to
int64 and avoid problems if GC64.

Since I've already checked the patch in, could you please
prepare a patch which will revert functional changes of 2/2
and use int64 instead?

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 0/2] Stacked diagnostic area follow-ups

[Nikita Pettik]

On 20 Apr 17:22, Kirill Yukhin wrote:
> Hello,
> 
> On 17 апр 23:16, Nikita Pettik wrote:
> > Branch: https://github.com/tarantool/tarantool/tree/np/gh-4887-ref-error-on-prev
> > Issue: https://github.com/tarantool/tarantool/issues/4887
> > 
> > Changes in v2:
> > 
> >  - modified test so that now it uses weak references to check that
> > gc collected error objects (i.e. there's no memory leaks);
> >  - added overflow check in error_ref() so that after 2^32 calls
> > of box.error.last() or error:prev() error object won't contain
> > broken reference counter.
> > 
> > Nikita Pettik (2):
> >   box/error: don't allow overflow of error ref counter
> >   box/error: ref error.prev while accessing it
> 
> I've checked your patchset into master.
> 
> However, looks like it'd be better to just replace int32 to
> int64 and avoid problems if GC64.

Could you please specify which problems you are talking about?
Current fix simply prevents from overflow, so it does not
introduce any other problems. Meanwhile users who ref single
error more than 2^32 times are likely doing smth wrong so
Lua error notifying them aboit it is a good practice IMHO.
 
> Since I've already checked the patch in, could you please
> prepare a patch which will revert functional changes of 2/2
> and use int64 instead?
> 
> --
> Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 0/2] Stacked diagnostic area follow-ups

[Kirill Yukhin]

On 20 апр 14:35, Nikita Pettik wrote:
> On 20 Apr 17:22, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 17 апр 23:16, Nikita Pettik wrote:
> > > Branch: https://github.com/tarantool/tarantool/tree/np/gh-4887-ref-error-on-prev
> > > Issue: https://github.com/tarantool/tarantool/issues/4887
> > > 
> > > Changes in v2:
> > > 
> > >  - modified test so that now it uses weak references to check that
> > > gc collected error objects (i.e. there's no memory leaks);
> > >  - added overflow check in error_ref() so that after 2^32 calls
> > > of box.error.last() or error:prev() error object won't contain
> > > broken reference counter.
> > > 
> > > Nikita Pettik (2):
> > >   box/error: don't allow overflow of error ref counter
> > >   box/error: ref error.prev while accessing it
> > 
> > I've checked your patchset into master.
> > 
> > However, looks like it'd be better to just replace int32 to
> > int64 and avoid problems if GC64.
> 
> Could you please specify which problems you are talking about?
> Current fix simply prevents from overflow, so it does not
> introduce any other problems. Meanwhile users who ref single
> error more than 2^32 times are likely doing smth wrong so
> Lua error notifying them aboit it is a good practice IMHO.

I'd avoid work 'likely' when it is possible. Vlad's approach
allows to do that: it will work always, wherever user doing
something right or wrong, it will just work.

BTW, we are close to start off adopotion of GC64 (w/ ujit or on
vanilla luajit), so ref error 2 billions times doesn't look
impossible.

--
Regards, Kirill Yukhin