From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 2875F445320 for ; Fri, 17 Jul 2020 06:09:22 +0300 (MSK) Date: Fri, 17 Jul 2020 06:08:34 +0300 From: Alexander Turenko Message-ID: <20200717030834.mlervru3vr4vfg7w@tkn_work_nb> References: <20200701203633.GA5559@tarantool.org> <20200716201057.67ity4dkaozagvjd@tkn_work_nb> <20200716214249.GA18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200716214249.GA18920@tarantool.org> 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy > Yes, Lua terms are too close and ambiguous: > * There is a "state" (, Lua universe) consisting > such global entities of runtime as GC state, registry, string table, > debug hooks, etc. > * There is a "thread" (, Lua coroutine) consisting > such coroutine-local entities as coroutine guest stack, top and base > slots of the current frame, reference to global state, etc. > > I'm totally fine with your wording now, but guess we already need kinda > glossary for internal usage :) It looks as the good candidate to include into Tarantool Internals set of articles [1] or to our GitHub wiki. [1]: https://tarantool-ref.readthedocs.io/en/latest/ > > > > + /* > > > > + * luaT_newthread() pops the new Lua state from > > > > + * tarantool_L and it is right thing to do: if we'll push > > > > + * something to it and yield, then another fiber will not > > > > + * know that a stack top is changed and may operate on a > > > > + * wrong slot. > > > > > > It seems to relate more to contract, so you can just > > > mention that it leaves no garbage on the given coroutine stack, ergo > > > nothing need to be popped in the caller function. > > > > I have two goals here: > > > > 1. Clarify luaT_newthread() contract on the caller side, because it is > > unusual for Lua. > > > > 2. Explain why we should not leave the new state on top of tarantool_L > > in luaT_temp_luastate(). > > > > There are two reasons, why leaving 'garbage' on tarantool_L is not > > acceptable here. I want to mention both here. > > > > I reformatted the comment a bit to make it more clear: > > > > | /* > > | * Unlike lua_newthread(), luaT_newthread() does not leave > > | * the new Lua state on tarantool_L. > > I was around to it today and unfortunately it does[1]. So you need to > explicitly pop a newly created coroutine from the guest stack right > after anchoring it to the registry. Ouch! I was sure that it does not leave the value... It seems I misread the source. Many thanks for catching this! I verified the actual behaviour, you're right: luaT_newthread() works just like lua_newthread(). There is luaL_ref() below, which pops the thread from tarantool_L, so the actual behaviour is correct. I moved and rephrased the comment: | /* Popped by luaL_ref(). */ | struct lua_State *L = luaT_newthread(tarantool_L); | if (L == NULL) | return NULL; | /* | * We should remove the reference to the newly created Lua | * thread from tarantool_L, because of two reasons: | * | * First, if we'll push something to tarantool_L and | * yield, then another fiber will not know that a stack | * top is changed and may operate on a wrong slot. | * | * Second, many requests that push a value to tarantool_L | * and yield may exhaust available slots on the stack. It | * is limited by LUAI_MAXSTACK build time constant (~65K). | * | * We cannot just pop the value, but should keep the | * reference in the registry while it is in use. | * Otherwise it may be garbage collected. | */ | *coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);