From 79ad5e86ccf33311585ceefa11acbc200780f16c Mon Sep 17 00:00:00 2001 From: Soner Tari Date: Fri, 15 Mar 2019 00:20:53 +0300 Subject: [PATCH] Fix expired conn handling, signal 6 crash: Do not lock conn thr mutex twice while freeing expired conns Fix passthrough mode: Do not SSL_free() srvdst ssl anymore and do not add conn to thr conns list twice --- protopassthrough.c | 1 - pxyconn.c | 13 +++++++++++-- pxyconn.h | 1 + pxythrmgr.c | 43 ++++++++++++++++++++++++++++++++++++++++--- pxythrmgr.h | 2 ++ 5 files changed, 54 insertions(+), 6 deletions(-) diff --git a/protopassthrough.c b/protopassthrough.c index ad47cc5..9426db2 100644 --- a/protopassthrough.c +++ b/protopassthrough.c @@ -76,7 +76,6 @@ protopassthrough_engage(pxy_conn_ctx_t *ctx) // @todo When we call bufferevent_free_and_close_fd(), connection stalls due to ssl shutdown? // We get srvdst writecb while ssl shutdown is still in progress, and srvdst readcb never fires - SSL_free(ctx->srvdst.ssl); ctx->srvdst.free(ctx->srvdst.bev, ctx); ctx->srvdst.bev = NULL; ctx->srvdst.ssl = NULL; diff --git a/pxyconn.c b/pxyconn.c index 1e5e584..20c861f 100644 --- a/pxyconn.c +++ b/pxyconn.c @@ -270,7 +270,11 @@ pxy_conn_ctx_free_child(pxy_conn_child_ctx_t *ctx) log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_conn_ctx_free_child: ENTER, child fd=%d, fd=%d\n", ctx->fd, ctx->conn->fd); #endif /* DEBUG_PROXY */ - pxy_thrmgr_detach_child(ctx->conn); + if (ctx->conn->detach_unlocked) { + pxy_thrmgr_detach_child(ctx->conn); + } else { + pxy_thrmgr_detach_child_locked(ctx->conn); + } // If the proto doesn't have special args, proto_free() callback is NULL if (ctx->protoctx->proto_free) { @@ -407,7 +411,12 @@ pxy_conn_ctx_free(pxy_conn_ctx_t *ctx, int by_requestor) } } - pxy_thrmgr_detach(ctx); + if (ctx->conn->detach_unlocked) { + pxy_thrmgr_detach(ctx); + } else { + pxy_thrmgr_detach_locked(ctx); + } + if (ctx->srchost_str) { free(ctx->srchost_str); } diff --git a/pxyconn.h b/pxyconn.h index 80c4df5..5d77fcf 100644 --- a/pxyconn.h +++ b/pxyconn.h @@ -292,6 +292,7 @@ struct pxy_conn_ctx { unsigned int sent_protoerror_msg : 1; unsigned int added_to_thr_conns : 1; + unsigned int detach_unlocked : 1; #ifdef HAVE_LOCAL_PROCINFO /* local process information */ diff --git a/pxythrmgr.c b/pxythrmgr.c index d5d53b7..5566166 100644 --- a/pxythrmgr.c +++ b/pxythrmgr.c @@ -257,6 +257,11 @@ pxy_thrmgr_timer_cb(UNUSED evutil_socket_t fd, UNUSED short what, log_dbg_level_printf(LOG_DBG_MODE_FINE, "pxy_thrmgr_timer_cb: Delete timed out conn thr=%d, fd=%d, child_fd=%d, at=%lld ct=%lld\n", expired->thr->thridx, expired->fd, expired->child_fd, (long long)(now - expired->atime), (long long)(now - expired->ctime)); #endif /* DEBUG_PROXY */ + + // We have already locked the thr mutex, do not lock again, otherwise we get signal 6 crash + // When detach_unlocked is set, *_ctx_free() functions call non-thread-safe detach functions + expired->detach_unlocked = 1; + // @attention Do not call the term function here, free the conn directly pxy_conn_free(expired, 1); ctx->timedout_conns++; @@ -458,6 +463,19 @@ pxy_thrmgr_free(pxy_thrmgr_ctx_t *ctx) void pxy_thrmgr_add_conn(pxy_conn_ctx_t *ctx) { + if (ctx->added_to_thr_conns) { + // Do not add conns twice + // While switching to passthrough mode, the conn must have already been added to its thread list by the previous proto +#ifdef DEBUG_PROXY + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_add_conn: Will not add conn twice, id=%llu, fd=%d\n", ctx->id, ctx->fd); +#endif /* DEBUG_PROXY */ + return; + } + +#ifdef DEBUG_PROXY + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_add_conn: Adding conn, id=%llu, fd=%d\n", ctx->id, ctx->fd); +#endif /* DEBUG_PROXY */ + pthread_mutex_lock(&ctx->thr->mutex); ctx->next = ctx->thr->conns; ctx->thr->conns = ctx; @@ -569,12 +587,25 @@ pxy_thrmgr_attach_child(pxy_conn_ctx_t *ctx) void pxy_thrmgr_detach(pxy_conn_ctx_t *ctx) { +#ifdef DEBUG_PROXY + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_detach: ENTER\n"); +#endif /* DEBUG_PROXY */ + assert(ctx->children == NULL); - pthread_mutex_lock(&ctx->thr->mutex); ctx->thr->load--; - if (ctx->added_to_thr_conns) + if (ctx->added_to_thr_conns) { pxy_thrmgr_remove_conn(ctx, &ctx->thr->conns); + // Shouldn't need to reset the added_to_thr_conns flag, because the conn ctx will be freed next, but just in case + ctx->added_to_thr_conns = 0; + } +} + +void +pxy_thrmgr_detach_locked(pxy_conn_ctx_t *ctx) +{ + pthread_mutex_lock(&ctx->thr->mutex); + pxy_thrmgr_detach(ctx); pthread_mutex_unlock(&ctx->thr->mutex); } @@ -585,8 +616,14 @@ pxy_thrmgr_detach_child(pxy_conn_ctx_t *ctx) log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_detach_child: ENTER\n"); #endif /* DEBUG_PROXY */ - pthread_mutex_lock(&ctx->thr->mutex); ctx->thr->load--; +} + +void +pxy_thrmgr_detach_child_locked(pxy_conn_ctx_t *ctx) +{ + pthread_mutex_lock(&ctx->thr->mutex); + pxy_thrmgr_detach_child(ctx); pthread_mutex_unlock(&ctx->thr->mutex); } diff --git a/pxythrmgr.h b/pxythrmgr.h index f13fd97..ab8f0f4 100644 --- a/pxythrmgr.h +++ b/pxythrmgr.h @@ -86,7 +86,9 @@ void pxy_thrmgr_add_conn(pxy_conn_ctx_t *) NONNULL(1); void pxy_thrmgr_attach(pxy_conn_ctx_t *) NONNULL(1); void pxy_thrmgr_attach_child(pxy_conn_ctx_t *) NONNULL(1); void pxy_thrmgr_detach(pxy_conn_ctx_t *) NONNULL(1); +void pxy_thrmgr_detach_locked(pxy_conn_ctx_t *) NONNULL(1); void pxy_thrmgr_detach_child(pxy_conn_ctx_t *) NONNULL(1); +void pxy_thrmgr_detach_child_locked(pxy_conn_ctx_t *) NONNULL(1); #endif /* !PXYTHRMGR_H */