Skip to content

Commit fefdae8

Browse files
Darleleta-denoyelle
authored andcommitted
BUG/MINOR: hlua: use CertCache.set() from various hlua contexts
Using CertCache.set() from init context wasn't explicitly supported and caused the process to crash: crash.lua: core.register_init(function() CertCache.set{filename="reg-tests/ssl/set_cafile_client.pem", ocsp=""} end) crash.conf: global lua-load crash.lua listen front bind localhost:9090 ssl crt reg-tests/ssl/set_cafile_client.pem ca-file reg-tests/ssl/set_cafile_interCA1.crt verify none ./haproxy -f crash.conf [NOTICE] (267993) : haproxy version is 3.0-dev2-640ff6-910 [NOTICE] (267993) : path to executable is ./haproxy [WARNING] (267993) : config : missing timeouts for proxy 'front'. | While not properly invalid, you will certainly encounter various problems | with such a configuration. To fix this, please ensure that all following | timeouts are set to a non-zero value: 'client', 'connect', 'server'. [1] 267993 segmentation fault (core dumped) ./haproxy -f crash.conf This is because in hlua_ckch_set/hlua_ckch_commit_yield, we always consider that we're being called from a yield-capable runtime context. As such, hlua_gethlua() is never checked for NULL and we systematically try to wake hlua->task and yield every 10 instances. In fact, if we're called from the body or init context (that is, during haproxy startup), hlua_gethlua() will return NULL, and in this case we shouldn't care about yielding because it is ok to commit all instances at once since haproxy is still starting up. Also, when calling CertCache.set() from a non-yield capable runtime context (such as hlua fetch context), we kept doing as if the yield succeeded, resulting in unexpected function termination (operation would be aborted and the CertCache lock wouldn't be released). Instead, now we explicitly state in the doc that CertCache.set() cannot be used from a non-yield capable runtime context, and we raise a runtime error if it is used that way. These bugs were discovered by reading the code when trying to address Svace report documented by @Bbulatov GH #2586. It should be backported up to 2.6 with 30fcca1 ("MINOR: ssl/lua: CertCache.set() allows to update an SSL certificate file") (cherry picked from commit 4f906a9) Signed-off-by: Amaury Denoyelle <[email protected]>
1 parent efc49d9 commit fefdae8

File tree

2 files changed

+29
-4
lines changed

2 files changed

+29
-4
lines changed

doc/lua-api/index.rst

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4463,6 +4463,10 @@ CertCache class
44634463
:param string certificate.issuer: The certificate of the OCSP issuer.
44644464
:param string certificate.sctl: An SCTL file.
44654465

4466+
.. Note::
4467+
This function may be slow. As such, it may only be used during startup
4468+
(main or init context) or from a yield-capable runtime context.
4469+
44664470
.. code-block:: lua
44674471
44684472
CertCache.set{filename="certs/localhost9994.pem.rsa", crt=crt}

src/hlua.c

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12989,8 +12989,10 @@ __LJMP static int hlua_ckch_commit_yield(lua_State *L, int status, lua_KContext
1298912989
list_for_each_entry_from(ckchi, &old_ckchs->ckch_inst, by_ckchs) {
1299012990
struct ckch_inst *new_inst;
1299112991

12992-
/* it takes a lot of CPU to creates SSL_CTXs, so we yield every 10 CKCH instances */
12993-
if (y % 10 == 0) {
12992+
/* it takes a lot of CPU to creates SSL_CTXs, so we yield every 10 CKCH instances
12993+
* during runtime
12994+
*/
12995+
if (hlua && (y % 10) == 0) {
1299412996

1299512997
*lua_ckchi = ckchi;
1299612998

@@ -13053,6 +13055,14 @@ __LJMP static int hlua_ckch_set(lua_State *L)
1305313055
WILL_LJMP(luaL_error(L, "'CertCache.set' needs a table as argument"));
1305413056

1305513057
hlua = hlua_gethlua(L);
13058+
if (hlua && HLUA_CANT_YIELD(hlua)) {
13059+
/* using hlua_ckch_set() during runtime from a context that
13060+
* doesn't allow yielding (e.g.: fetches) is not supported
13061+
* as it may cause contention.
13062+
*/
13063+
WILL_LJMP(luaL_error(L, "Cannot use CertCache.set from a "
13064+
"non-yield capable runtime context"));
13065+
}
1305613066

1305713067
/* FIXME: this should not return an error but should come back later */
1305813068
if (HA_SPIN_TRYLOCK(CKCH_LOCK, &ckch_lock))
@@ -13135,8 +13145,19 @@ __LJMP static int hlua_ckch_set(lua_State *L)
1313513145
lua_ckchi = lua_newuserdata(L, sizeof(struct ckch_inst *));
1313613146
*lua_ckchi = NULL;
1313713147

13138-
task_wakeup(hlua->task, TASK_WOKEN_MSG);
13139-
MAY_LJMP(hlua_yieldk(L, 0, 0, hlua_ckch_commit_yield, TICK_ETERNITY, 0));
13148+
if (hlua) {
13149+
/* yield right away to let hlua_ckch_commit_yield() benefit from
13150+
* a fresh task cycle on next wakeup
13151+
*/
13152+
task_wakeup(hlua->task, TASK_WOKEN_MSG);
13153+
MAY_LJMP(hlua_yieldk(L, 0, 0, hlua_ckch_commit_yield, TICK_ETERNITY, 0));
13154+
} else {
13155+
/* body/init context: yielding not available, perform the commit as a
13156+
* 1-shot operation (may be slow, but haproxy process is starting so
13157+
* it is acceptable)
13158+
*/
13159+
hlua_ckch_commit_yield(L, LUA_OK, 0);
13160+
}
1314013161

1314113162
end:
1314213163
HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);

0 commit comments

Comments
 (0)