[Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Alexander Turenko]

> Yes, Lua terms are too close and ambiguous:
> * There is a "state" (<struct global_State>, Lua universe) consisting
>   such global entities of runtime as GC state, registry, string table,
>   debug hooks, etc.
> * There is a "thread" (<strict lua_State>, 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 <luaT_newthread> 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);