BUG/MEDIUM: hlua: prevent deadlocks with main lua lock
Main lua lock is used at various places in the code.
Most of the time it is used from unprotected lua environments,
in which case the locking is mandatory.
But there are some cases where the lock is attempted from protected
lua environments, meaning that lock is already owned by the current
thread. Thus new locking attempt should be skipped to prevent any
deadlocks from occuring.
To address this, "already_safe" lock hint was implemented in
hlua_ctx_init() function with commit bf90ce1
("BUG/MEDIUM: lua: dead lock when Lua tasks are trigerred")
But this approach is not very safe, for 2 reasons:
First reason is that there are still some code paths that could lead
to deadlocks.
For instance, in register_task(), hlua_ctx_init() is called with
already_safe set to 1 to prevent deadlock from occuring.
But in case of task init failure, hlua_ctx_destroy() will be called
from the same environment (protected environment), and hlua_ctx_destroy()
does not offer the already_safe lock hint.. resulting in a deadlock.
Second reason is that already_safe hint is used to completely skip
SET_LJMP macros (which manipulates the lock internally), resulting
in some logics in the function being unprotected from lua aborts in
case of unexpected errors when manipulating the lua stack (the lock
does not protect against longjmps)
Instead of leaving the locking responsibility to the caller, which is
quite error prone since we must find out ourselves if we are or not in
a protected environment (and is not robust against code re-use),
we move the deadlock protection logic directly in hlua_lock() function.
Thanks to a thread-local lock hint, we can easily guess if the current
thread already owns the main lua lock, in which case the locking attempt
is skipped. The thread-local lock hint is implemented as a counter so
that the lock is properly dropped when the counter reaches 0.
(to match actual lock() and unlock() calls)
This commit depends on "MINOR: hlua: simplify lua locking"
It may be backported to every stable versions.
[prior to 2.5 lua filter API did not exist, filter-related parts
should be skipped]
diff --git a/src/hlua.c b/src/hlua.c
index 7704d17..19f848a 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -162,15 +162,33 @@
HA_SPIN_UNLOCK(LUA_LOCK, &hlua_global_lock);
}
-/* lua lock helpers: only lock when required */
+/* lua lock helpers: only lock when required
+ *
+ * state_id == 0: we're operating on the main lua stack (shared between
+ * os threads), so we need to acquire the main lock
+ *
+ * If the thread already owns the lock (_hlua_locked != 0), skip the lock
+ * attempt. This could happen if we run under protected lua environment.
+ * Not doing this could result in deadlocks because of nested locking
+ * attempts from the same thread
+ */
+static THREAD_LOCAL int _hlua_locked = 0;
static inline void hlua_lock(struct hlua *hlua)
{
- if (hlua->state_id == 0)
+ if (hlua->state_id != 0)
+ return;
+ if (!_hlua_locked)
lua_take_global_lock();
+ _hlua_locked += 1;
}
static inline void hlua_unlock(struct hlua *hlua)
{
- if (hlua->state_id == 0)
+ if (hlua->state_id != 0)
+ return;
+ BUG_ON(_hlua_locked <= 0);
+ _hlua_locked--;
+ /* drop the lock once the lock count reaches 0 */
+ if (!_hlua_locked)
lua_drop_global_lock();
}
@@ -1277,21 +1295,12 @@
* initialisation fails (example: out of memory error), the lua function
* throws an error (longjmp).
*
- * In some case (at least one), this function can be called from safe
- * environment, so we must not initialise it. While the support of
- * threads appear, the safe environment set a lock to ensure only one
- * Lua execution at a time. If we initialize safe environment in another
- * safe environment, we have a dead lock.
- *
- * set "already_safe" true if the context is initialized form safe
- * Lua function.
- *
* This function manipulates two Lua stacks: the main and the thread. Only
* the main stack can fail. The thread is not manipulated. This function
* MUST NOT manipulate the created thread stack state, because it is not
* protected against errors thrown by the thread stack.
*/
-int hlua_ctx_init(struct hlua *lua, int state_id, struct task *task, int already_safe)
+int hlua_ctx_init(struct hlua *lua, int state_id, struct task *task)
{
lua->Mref = LUA_REFNIL;
lua->flags = 0;
@@ -1300,24 +1309,20 @@
lua->state_id = state_id;
LIST_INIT(&lua->com);
MT_LIST_INIT(&lua->hc_list);
- if (!already_safe) {
- if (!SET_SAFE_LJMP_PARENT(lua)) {
- lua->Tref = LUA_REFNIL;
- return 0;
- }
+ if (!SET_SAFE_LJMP_PARENT(lua)) {
+ lua->Tref = LUA_REFNIL;
+ return 0;
}
lua->T = lua_newthread(hlua_states[state_id]);
if (!lua->T) {
lua->Tref = LUA_REFNIL;
- if (!already_safe)
- RESET_SAFE_LJMP_PARENT(lua);
+ RESET_SAFE_LJMP_PARENT(lua);
return 0;
}
hlua_sethlua(lua);
lua->Tref = luaL_ref(hlua_states[state_id], LUA_REGISTRYINDEX);
lua->task = task;
- if (!already_safe)
- RESET_SAFE_LJMP_PARENT(lua);
+ RESET_SAFE_LJMP_PARENT(lua);
return 1;
}
@@ -8723,7 +8728,7 @@
task->context = hlua;
task->process = hlua_process_task;
- if (!hlua_ctx_init(hlua, state_id, task, 1))
+ if (!hlua_ctx_init(hlua, state_id, task))
goto alloc_error;
/* Restore the function in the stack. */
@@ -8774,7 +8779,7 @@
}
HLUA_INIT(hlua);
stream->hlua = hlua;
- if (!hlua_ctx_init(stream->hlua, fcn_ref_to_stack_id(fcn), stream->task, 0)) {
+ if (!hlua_ctx_init(stream->hlua, fcn_ref_to_stack_id(fcn), stream->task)) {
SEND_ERR(stream->be, "Lua converter '%s': can't initialize Lua context.\n", fcn->name);
return 0;
}
@@ -8911,7 +8916,7 @@
}
hlua->T = NULL;
stream->hlua = hlua;
- if (!hlua_ctx_init(stream->hlua, fcn_ref_to_stack_id(fcn), stream->task, 0)) {
+ if (!hlua_ctx_init(stream->hlua, fcn_ref_to_stack_id(fcn), stream->task)) {
SEND_ERR(stream->be, "Lua sample-fetch '%s': can't initialize Lua context.\n", fcn->name);
return 0;
}
@@ -9253,7 +9258,7 @@
}
HLUA_INIT(hlua);
s->hlua = hlua;
- if (!hlua_ctx_init(s->hlua, fcn_ref_to_stack_id(rule->arg.hlua_rule->fcn), s->task, 0)) {
+ if (!hlua_ctx_init(s->hlua, fcn_ref_to_stack_id(rule->arg.hlua_rule->fcn), s->task)) {
SEND_ERR(px, "Lua action '%s': can't initialize Lua context.\n",
rule->arg.hlua_rule->fcn->name);
goto end;
@@ -9440,7 +9445,7 @@
* permits to save performances because a systematic
* Lua initialization cause 5% performances loss.
*/
- if (!hlua_ctx_init(hlua, fcn_ref_to_stack_id(ctx->rule->arg.hlua_rule->fcn), task, 0)) {
+ if (!hlua_ctx_init(hlua, fcn_ref_to_stack_id(ctx->rule->arg.hlua_rule->fcn), task)) {
SEND_ERR(strm->be, "Lua applet tcp '%s': can't initialize Lua context.\n",
ctx->rule->arg.hlua_rule->fcn->name);
return -1;
@@ -9631,7 +9636,7 @@
* permits to save performances because a systematic
* Lua initialization cause 5% performances loss.
*/
- if (!hlua_ctx_init(hlua, fcn_ref_to_stack_id(ctx->rule->arg.hlua_rule->fcn), task, 0)) {
+ if (!hlua_ctx_init(hlua, fcn_ref_to_stack_id(ctx->rule->arg.hlua_rule->fcn), task)) {
SEND_ERR(strm->be, "Lua applet http '%s': can't initialize Lua context.\n",
ctx->rule->arg.hlua_rule->fcn->name);
return -1;
@@ -10270,7 +10275,7 @@
ctx->task->process = hlua_applet_wakeup;
/* Initialises the Lua context */
- if (!hlua_ctx_init(hlua, fcn_ref_to_stack_id(fcn), ctx->task, 0)) {
+ if (!hlua_ctx_init(hlua, fcn_ref_to_stack_id(fcn), ctx->task)) {
SEND_ERR(NULL, "Lua cli '%s': can't initialize Lua context.\n", fcn->name);
goto error;
}
@@ -10706,7 +10711,7 @@
}
HLUA_INIT(hlua);
s->hlua = hlua;
- if (!hlua_ctx_init(s->hlua, reg_flt_to_stack_id(conf->reg), s->task, 0)) {
+ if (!hlua_ctx_init(s->hlua, reg_flt_to_stack_id(conf->reg), s->task)) {
SEND_ERR(s->be, "Lua filter '%s': can't initialize Lua context.\n",
conf->reg->name);
ret = 0;
@@ -10731,8 +10736,8 @@
}
HLUA_INIT(flt_ctx->hlua[0]);
HLUA_INIT(flt_ctx->hlua[1]);
- if (!hlua_ctx_init(flt_ctx->hlua[0], reg_flt_to_stack_id(conf->reg), s->task, 0) ||
- !hlua_ctx_init(flt_ctx->hlua[1], reg_flt_to_stack_id(conf->reg), s->task, 0)) {
+ if (!hlua_ctx_init(flt_ctx->hlua[0], reg_flt_to_stack_id(conf->reg), s->task) ||
+ !hlua_ctx_init(flt_ctx->hlua[1], reg_flt_to_stack_id(conf->reg), s->task)) {
SEND_ERR(s->be, "Lua filter '%s': can't initialize filter Lua context.\n",
conf->reg->name);
ret = 0;