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

[Igor Munkin <imun@tarantool.org>]

Sasha,

Thanks for the updates!

On 16.07.20, Alexander Turenko wrote:

<snipped>

> > > +/**
> > > + * Get a temporary Lua state.
> > > + *
> > > + * Use case: a function does not accept a Lua state as an argument
> > > + * to allow using from C code, but uses a Lua value, which is
> > > + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed
> > > + * to get and process the value.
> > > + *
> > > + * The returned state shares LUA_REGISTRYINDEX with `tarantool_L`.
> > 
> > Pardon, I don't get this line.
> 
> I made an attempt to clarify the comment:
> 
>  | * The resulting Lua state has a separate Lua stack, but the same
>  | * globals and registry as `tarantool_L` (and all Lua states in
>  | * tarantool at the moment of writing this).

Neat, thanks!

> 
> Existing terms are not perfect: both lua_newthread() and luaL_newstate()
> return the same type of pointer: <struct lua_State *>. So I called any
> so typed structure 'Lua state' or just 'state'. I hope now the idea 'you
> can reference something in a registry of some other Lua state and then
> get it from the registry using this temporary Lua state' is clear.

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 :)

> 

<snipped>

> 
> > > +	/*
> > > +	 * 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.

>  |  *
>  |  * It is desired behaviour here, 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).
>  |  */
> 
> > > +	 *
> > > +	 * Second, many requests that push a value to tarantool_L
> > > +	 * and yield may exhaust available slots on the stack.
> > 
> > Pardon, I don't get this line.
> 
> I don't know how clarify it and don't make it huge.
> 
> The case is the following: tarantool serves many similar requests. All
> requests execute luaT_temp_luastate() function and call a merge source
> fetch function (an iterator generator), which yields. If we would leave
> new Lua states on tarantool_L, it would be possible that all available
> stack slots in tarantool_L are occupied and the next request will fails
> with the 'stack overflow' error.
> 
> After offline discussion with Igor we agreed to clarify the comment with
> mention of the LUAI_MAXSTACK constant. The new comment is the following:
> 
>  | * 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).

That's more than enough, thanks!

> 

<snipped>

[1]: https://github.com/tarantool/tarantool/blob/master/src/lua/utils.h#L617

-- 
Best regards,
IM