[tarantool-patches] [PATCH] Fix premature cdata collecting in luaT_pusherror()

[tarantool-patches] [PATCH] Fix premature cdata collecting in luaT_pusherror()

[Alexander Turenko]

This is follow up of 28c7e667aee9be8c3288597bcc179d9b4e7b4940 to fix
luaT_pusherror() itself, not only luaT_error().

Fixes #1955 (again).
---

https://github.com/tarantool/tarantool/issues/1955
https://github.com/tarantool/tarantool/tree/Totktonada/gh-1955-fix-luaT_pusherror

 src/lua/error.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/lua/error.c b/src/lua/error.c
index 71345927d..fca87f9d6 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -75,12 +75,20 @@ luaL_error_gc(struct lua_State *L)
 void
 luaT_pusherror(struct lua_State *L, struct error *e)
 {
+	/*
+	 * gh-1955 luaT_pusherror allocates Lua objects, thus it
+	 * may trigger GC. GC may invoke finalizers which are
+	 * arbitrary Lua code, potentially invalidating last error
+	 * object, hence error_ref below.
+	 *
+	 * It also important to reference the error first and only
+	 * then set the finalizer.
+	 */
+	error_ref(e);
 	assert(CTID_CONST_STRUCT_ERROR_REF != 0);
 	struct error **ptr = (struct error **)
 		luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
 	*ptr = e;
-	/* The order is important - first reference the error, then set gc */
-	error_ref(e);
 	lua_pushcfunction(L, luaL_error_gc);
 	luaL_setcdatagc(L, -2);
 }
@@ -90,15 +98,7 @@ luaT_error(lua_State *L)
 {
 	struct error *e = diag_last_error(&fiber()->diag);
 	assert(e != NULL);
-	/*
-	 * gh-1955 luaT_pusherror allocates Lua objects, thus it may trigger
-	 * GC. GC may invoke finalizers which are arbitrary Lua code,
-	 * potentially invalidating last error object, hence error_ref
-	 * below.
-	 */
-	error_ref(e);
 	luaT_pusherror(L, e);
-	error_unref(e);
 	lua_error(L);
 	unreachable();
 	return 0;
-- 
2.19.2

[tarantool-patches] Re: [PATCH] Fix premature cdata collecting in luaT_pusherror()

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [18/12/07 18:38]:
> This is follow up of 28c7e667aee9be8c3288597bcc179d9b4e7b4940 to fix
> luaT_pusherror() itself, not only luaT_error().
> 
> Fixes #1955 (again).

OK to push. Please use cherry-picking or submit a separate patch
for 1.10.

Branches to fix: 2.1, 1.10


Thanks,

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] [PATCH] Fix premature cdata collecting in luaT_pusherror()

[Alexander Turenko]

This is follow up of 28c7e667aee9be8c3288597bcc179d9b4e7b4940 to fix
luaT_pusherror() itself, not only luaT_error().

Fixes #1955 (again).

It is backport of 480868ff8f304ead2fd03e317902a1e8d41c6248 for 1.10.
---

https://github.com/tarantool/tarantool/issues/1955
https://github.com/tarantool/tarantool/tree/Totktonada/gh-1955-fix-luaT_pusherror-1.10

 src/lua/utils.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 27772a9d3..b05d16a73 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -898,12 +898,20 @@ luaL_error_gc(struct lua_State *L)
 void
 luaT_pusherror(struct lua_State *L, struct error *e)
 {
+	/*
+	 * gh-1955 luaT_pusherror allocates Lua objects, thus it
+	 * may trigger GC. GC may invoke finalizers which are
+	 * arbitrary Lua code, potentially invalidating last error
+	 * object, hence error_ref below.
+	 *
+	 * It also important to reference the error first and only
+	 * then set the finalizer.
+	 */
+	error_ref(e);
 	assert(CTID_CONST_STRUCT_ERROR_REF != 0);
 	struct error **ptr = (struct error **) luaL_pushcdata(L,
 		CTID_CONST_STRUCT_ERROR_REF);
 	*ptr = e;
-	/* The order is important - first reference the error, then set gc */
-	error_ref(e);
 	lua_pushcfunction(L, luaL_error_gc);
 	luaL_setcdatagc(L, -2);
 }
@@ -913,15 +921,7 @@ luaT_error(lua_State *L)
 {
 	struct error *e = diag_last_error(&fiber()->diag);
 	assert(e != NULL);
-	/*
-	 * gh-1955 luaT_pusherror allocates Lua objects, thus it may trigger
-	 * GC. GC may invoke finalizers which are arbitrary Lua code,
-	 * potentially invalidating last error object, hence error_ref
-	 * below.
-	 */
-	error_ref(e);
 	luaT_pusherror(L, e);
-	error_unref(e);
 	lua_error(L);
 	unreachable();
 	return 0;
-- 
2.19.2

[tarantool-patches] Re: [PATCH] Fix premature cdata collecting in luaT_pusherror()

[Vladislav Shpilevoy]

On 07/12/2018 21:15, Alexander Turenko wrote:
> This is follow up of 28c7e667aee9be8c3288597bcc179d9b4e7b4940 to fix
> luaT_pusherror() itself, not only luaT_error().
> 
> Fixes #1955 (again).
> 
> It is backport of 480868ff8f304ead2fd03e317902a1e8d41c6248 for 1.10.
> ---
> 
> https://github.com/tarantool/tarantool/issues/1955
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-1955-fix-luaT_pusherror-1.10
> 
>   src/lua/utils.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 27772a9d3..b05d16a73 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -898,12 +898,20 @@ luaL_error_gc(struct lua_State *L)
>   void
>   luaT_pusherror(struct lua_State *L, struct error *e)
>   {
> +	/*
> +	 * gh-1955 luaT_pusherror allocates Lua objects, thus it

Sorry, but usually we do not mention issue numbers in comments.
Please, reword the comment so as to do not use explicit issue
number.

> +	 * may trigger GC. GC may invoke finalizers which are
> +	 * arbitrary Lua code, potentially invalidating last error
> +	 * object, hence error_ref below.
> +	 *
> +	 * It also important to reference the error first and only
> +	 * then set the finalizer.
> +	 */
> +	error_ref(e);
>   	assert(CTID_CONST_STRUCT_ERROR_REF != 0);
>   	struct error **ptr = (struct error **) luaL_pushcdata(L,
>   		CTID_CONST_STRUCT_ERROR_REF);
>   	*ptr = e;
> -	/* The order is important - first reference the error, then set gc */
> -	error_ref(e);
>   	lua_pushcfunction(L, luaL_error_gc);
>   	luaL_setcdatagc(L, -2);
>   }
> @@ -913,15 +921,7 @@ luaT_error(lua_State *L)
>   {
>   	struct error *e = diag_last_error(&fiber()->diag);
>   	assert(e != NULL);
> -	/*
> -	 * gh-1955 luaT_pusherror allocates Lua objects, thus it may trigger
> -	 * GC. GC may invoke finalizers which are arbitrary Lua code,
> -	 * potentially invalidating last error object, hence error_ref
> -	 * below.
> -	 */
> -	error_ref(e);
>   	luaT_pusherror(L, e);
> -	error_unref(e);
>   	lua_error(L);
>   	unreachable();
>   	return 0;
> 

[tarantool-patches] Re: [PATCH] Fix premature cdata collecting in luaT_pusherror()

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/08 01:20]:
> 
> 
> On 07/12/2018 21:15, Alexander Turenko wrote:
> > This is follow up of 28c7e667aee9be8c3288597bcc179d9b4e7b4940 to fix
> > luaT_pusherror() itself, not only luaT_error().
> > 
> > Fixes #1955 (again).
> > 
> > It is backport of 480868ff8f304ead2fd03e317902a1e8d41c6248 for 1.10.
> > ---
> > 
> > https://github.com/tarantool/tarantool/issues/1955
> > https://github.com/tarantool/tarantool/tree/Totktonada/gh-1955-fix-luaT_pusherror-1.10
> > 
> >   src/lua/utils.c | 20 ++++++++++----------
> >   1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/lua/utils.c b/src/lua/utils.c
> > index 27772a9d3..b05d16a73 100644
> > --- a/src/lua/utils.c
> > +++ b/src/lua/utils.c
> > @@ -898,12 +898,20 @@ luaL_error_gc(struct lua_State *L)
> >   void
> >   luaT_pusherror(struct lua_State *L, struct error *e)
> >   {
> > +	/*
> > +	 * gh-1955 luaT_pusherror allocates Lua objects, thus it
> 
> Sorry, but usually we do not mention issue numbers in comments.
> Please, reword the comment so as to do not use explicit issue
> number.

I think it's oK to mention issue ## in a changeset doesn't have a test case.

This all comes from future use: if there is no test, it's
difficult to understand what was the patch about without a quick
link to the bugs database.

> > -	/*
> > -	 * gh-1955 luaT_pusherror allocates Lua objects, thus it may trigger
> > -	 * GC. GC may invoke finalizers which are arbitrary Lua code,
> > -	 * potentially invalidating last error object, hence error_ref
> > -	 * below.
> > -	 */

Alexander only moved this comment. The comment itself was added by
Nick Zavaritsky, I think it was appropriate in this case.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH] Fix premature cdata collecting in luaT_pusherror()

[Vladimir Davydov]

On Fri, Dec 07, 2018 at 06:16:28PM +0300, Alexander Turenko wrote:
> This is follow up of 28c7e667aee9be8c3288597bcc179d9b4e7b4940 to fix
> luaT_pusherror() itself, not only luaT_error().
> 
> Fixes #1955 (again).
> ---
> 
> https://github.com/tarantool/tarantool/issues/1955
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-1955-fix-luaT_pusherror
> 
>  src/lua/error.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Pushed to 2.1 and 1.10.