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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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