From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 4AC8142EF5C for ; Fri, 19 Jun 2020 01:48:42 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <811cb85f-f9e5-f353-68bb-010081d85eb2@tarantool.org> Date: Fri, 19 Jun 2020 00:48:40 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko , Igor Munkin Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! > diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c > index 1b155152b..cc5626cbc 100644 > --- a/src/box/lua/merger.c > +++ b/src/box/lua/merger.c > @@ -396,16 +464,12 @@ luaL_merge_source_buffer_new(struct lua_State *L) > } > > /** > - * Call a user provided function to get a next data chunk (a > - * buffer). > - * > - * Return 1 when a new buffer is received, 0 when a buffers > - * iterator ends and -1 at error and set a diag. > + * Helper for `luaL_merge_source_buffer_fetch()`. > */ > static int > -luaL_merge_source_buffer_fetch(struct merge_source_buffer *source) > +luaL_merge_source_buffer_fetch_impl(struct lua_State *L, > + struct merge_source_buffer *source) I see my comment to the former first commit was missed somewhy, so I say it here: - If a function takes lua_State, it is not mean it should get luaL_ prefix. Since the first commit was dropped, this comment we can't fix. - If a function is a method of struct, struct pointer goes *always* first, like 'this' argument in C++. Present of lua_State does not mean it should be first. This can and should be fixed, I think. This function in the first place is a method of merge_source_buffer. So it should take this argument first. luaL_merge_source_buffer_fetch_impl() currently is the only method of merge_source_buffer, which takes merge_source_buffer not in the first argument. It means, I don't see how the consistency is improved here. The same for luaL_merge_source_table_fetch, luaL_merge_source_table_next_impl, luaL_merge_source_tuple_fetch, luaL_merge_source_tuple_next_impl.