From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
 bits)) (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 12FDD42EF5C
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 19 Jun 2020 01:47:47 +0300 (MSK)
References: <cover.1591028838.git.alexander.turenko@tarantool.org>
 <0f0ad73e2fce564e22dcc8f9970d8aedd028c279.1591028838.git.alexander.turenko@tarantool.org>
 <920c35d4-150b-bae5-345f-e819ada02367@tarantool.org>
 <20200607165816.4r5s7ah4pa5uihyl@tkn_work_nb>
 <c00b3856-7938-5d8b-e51a-279f1def539d@tarantool.org>
 <20200617175339.aieq23vkcdqu4z7u@tkn_work_nb>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <b2abbbee-207d-08e8-8ba1-3e26f1d4789a@tarantool.org>
Date: Fri, 19 Jun 2020 00:47:46 +0200
MIME-Version: 1.0
In-Reply-To: <20200617175339.aieq23vkcdqu4z7u@tkn_work_nb>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 2/3] merger: fix NULL dereference
 when called via iproto
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Hi! Thanks for the fixes!

On 17/06/2020 19:53, Alexander Turenko wrote:
>>>     Usage of the fiber-local Lua state is not quite correct now: merge
>>>     source code may left garbage on a stack in case of failures (like
>>
>> 1. merge -> merger.
> 
> I use term 'merge source' for sources, we agreed on it with Vladimir D.,
> when the merger was designed. Removed 'code' to don't confuse a reader:
> 'merge source code' -> 'a merge source'.

Exactly. I read it as 'source code', not 'merge source'. Now it is fine.

>>> +	struct lua_State *L = luaT_newthread(tarantool_L);
>>> +	if (L == NULL)
>>> +		return NULL;
>>
>> 2. luaT_newthread() does not set a diag. That may lead to a crash,
>> because as far as I see, this function may be called
>> lbox_merge_source_gen() indirectly, somewhere deep in the callstack.
>> And it luaT_error(), when merge_source_next() fails.
> 
> As I see, it is not so.
> 
> luaT_newthread_wrapper() may raise a Lua error (only 'not enough memory'
> I guess), luaT_cpcall() calls luaT_toerror() in this case, which invokes
> diag_set(LuajitError, <...>).
> 
> Isn't I miss something?

You are right, all is fine here.