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

[Tarantool-patches] [PATCH] 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
---
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

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 minor comments below.

> 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)

1. To reduce diff size and to keep inlining when
possible you can remove word 'static', and add

    extern inline void
    error_unref(struct error *e);

to diag.c.

Yeah, exactly 'extern inline', not just 'extern'.
I recently discovered that it is a special thing for
such cases.

> -{
> -	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);
>  
> 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)
> + | ...
> +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')

2. Did you check that preve does not leak when GC starts working?
You can use setmetatable({}, {__mode = 'v'}) for that, see
examples by grepping " __mode = 'v' ".

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

[Nikita Pettik]

On 16 Apr 02:19, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 2 minor comments below.
> 
> > 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)
> 
> 1. To reduce diff size and to keep inlining when
> possible you can remove word 'static', and add
> 
>     extern inline void
>     error_unref(struct error *e);
> 
> to diag.c.

error_unref() doesn't seem to be small enough to keep
it inlining after diagnostic area introduction, so I'd
better remove inline attribute anyway.
 
> Yeah, exactly 'extern inline', not just 'extern'.
> I recently discovered that it is a special thing for
> such cases.

Thanks, will keep it in mind.

> > 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)
> > + | ...
> > +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')
> 
> 2. Did you check that preve does not leak when GC starts working?
> You can use setmetatable({}, {__mode = 'v'}) for that, see
> examples by grepping " __mode = 'v' ".

Yep, I verified it using prints since I didn't know about
weak references in Lua. Thanks, I've extended test. Diff:

diff --git a/test/box/error.result b/test/box/error.result
index 49e35b942..ee9dc2a85 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -860,6 +860,9 @@ ok, err = test_func()
 preve = err.prev
  | ---
  | ...
+gc_err = setmetatable({preve}, {__mode = 'v'})
+ | ---
+ | ...
 err:set_prev(nil)
  | ---
  | ...
@@ -867,10 +870,27 @@ err.prev
  | ---
  | - null
  | ...
-preve
+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
  | ---
- | - '[string "return function(tuple) local json = require(''..."]:1: attempt to call
- |   global ''require'' (a nil value)'
+ | - []
  | ...
 
 s:drop()
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index dbc4d143f..3ae197379 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -242,9 +242,16 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri
 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
-preve
+collectgarbage('collect')
+--  Still one reference to err.prev so it should not be collected.
+--
+gc_err
+preve = nil
+collectgarbage('collect')
+gc_err

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

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

Now there is a new problem:

    value = 2147483647 + 100
    e1 = box.error.new(1000, 'Message1')
    e2 = box.error.new(1001, 'Message2')
    e1:set_prev(e2)
    prev = nil
    for i = 1, value do prev = e1.prev end

    tarantool> prev._refs
    ---
    - -2147483547
    ...

Negative ref count.

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

[Nikita Pettik]

On 16 Apr 23:17, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> Now there is a new problem:
> 
>     value = 2147483647 + 100
>     e1 = box.error.new(1000, 'Message1')
>     e2 = box.error.new(1001, 'Message2')
>     e1:set_prev(e2)
>     prev = nil
>     for i = 1, value do prev = e1.prev end

Oh, if user wrote code like this I guess negative reference
counter would be fair panishment.

Still ref counter overflow can be achieved without stacked diag:
box.error.last() refs counter as well. So I introduced overflow
check in error_ref() in a separate patch.

 
>     tarantool> prev._refs
>     ---
>     - -2147483547
>     ...
> 
> Negative ref count.

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

[Vladislav Shpilevoy]

On 17/04/2020 22:06, Nikita Pettik wrote:
> On 16 Apr 23:17, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the fixes!
>>
>> Now there is a new problem:
>>
>>     value = 2147483647 + 100
>>     e1 = box.error.new(1000, 'Message1')
>>     e2 = box.error.new(1001, 'Message2')
>>     e1:set_prev(e2)
>>     prev = nil
>>     for i = 1, value do prev = e1.prev end
> 
> Oh, if user wrote code like this I guess negative reference
> counter would be fair panishment.

Actually there was a real problem with that, but with tuples.
They have uint32 ref counter, and it really can overflow in Lua
sometimes, when GC is too lazy, and the same tuple is got
many many times from space:get(). That was fixed by separating
one bit as a flag of having an external 'big-ref' uint64 number.

So a user won't write exactly the same stupid code, but it
achievable implicitly, so as you won't even notice.