From bf67b617c296d9b13eb439b53ad20b7cedffec15 Mon Sep 17 00:00:00 2001 From: Soner Tari Date: Fri, 22 Mar 2019 15:21:39 +0300 Subject: [PATCH] Keep track of ssl conns waiting for the first packet, and remove them if they time out Otherwise if no packet arrives, hence readcb does not fire, that ssl conn is lost causing memory and fd leak Accepting a connection does not mean that a packet will be received Use better names --- protossl.c | 3 ++ pxyconn.c | 8 ++--- pxyconn.h | 5 +++ pxythrmgr.c | 102 ++++++++++++++++++++++++++++++++++++++++++++-------- pxythrmgr.h | 9 +++-- 5 files changed, 106 insertions(+), 21 deletions(-) diff --git a/protossl.c b/protossl.c index d87d6e4..f2a973a 100644 --- a/protossl.c +++ b/protossl.c @@ -953,9 +953,12 @@ protossl_fd_readcb(MAYBE_UNUSED evutil_socket_t fd, UNUSED short what, void *arg goto out; if (event_add(ctx->ev, NULL) == -1) goto out; + pxy_thrmgr_add_pending_ssl_conn(ctx); return; } + pxy_thrmgr_remove_pending_ssl_conn(ctx); + // Child connections will use the sni info obtained by the parent conn /* for SSL, peek ClientHello and parse SNI from it */ diff --git a/pxyconn.c b/pxyconn.c index ffb6d31..282802b 100644 --- a/pxyconn.c +++ b/pxyconn.c @@ -265,9 +265,9 @@ pxy_conn_ctx_free_child(pxy_conn_child_ctx_t *ctx) #endif /* DEBUG_PROXY */ if (ctx->conn->thr_locked) { - pxy_thrmgr_detach_child(ctx->conn); + pxy_thrmgr_detach_child_unlocked(ctx->conn); } else { - pxy_thrmgr_detach_child_locked(ctx->conn); + pxy_thrmgr_detach_child(ctx->conn); } // If the proto doesn't have special args, proto_free() callback is NULL @@ -419,9 +419,9 @@ pxy_conn_ctx_free(pxy_conn_ctx_t *ctx, int by_requestor) } if (ctx->conn->thr_locked) { - pxy_thrmgr_detach(ctx); + pxy_thrmgr_detach_unlocked(ctx); } else { - pxy_thrmgr_detach_locked(ctx); + pxy_thrmgr_detach(ctx); } if (ctx->srchost_str) { diff --git a/pxyconn.h b/pxyconn.h index 655708f..d85d2f8 100644 --- a/pxyconn.h +++ b/pxyconn.h @@ -132,6 +132,11 @@ struct ssl_ctx { char *srvdst_ssl_version; char *srvdst_ssl_cipher; + + // Per-thread list of ssl conns waiting for the first read event + // Note that accepting a connection does not mean that a packet will be received + pxy_conn_ctx_t *next_pending; + unsigned int pending : 1; /* 1 until first readcb */ }; struct proto_ctx { diff --git a/pxythrmgr.c b/pxythrmgr.c index c17c33b..0c29753 100644 --- a/pxythrmgr.c +++ b/pxythrmgr.c @@ -66,6 +66,16 @@ pxy_thrmgr_get_thr_expired_conns(pxy_thr_ctx_t *tctx, pxy_conn_ctx_t **expired_c ctx = ctx->next; } + ctx = tctx->pending_ssl_conns; + while (ctx) { + time_t elapsed_time = now - ctx->atime; + if (elapsed_time > (time_t)tctx->thrmgr->opts->conn_idle_timeout) { + ctx->next_expired = *expired_conns; + *expired_conns = ctx; + } + ctx = ctx->sslctx->next_pending; + } + if (tctx->thrmgr->opts->statslog) { ctx = *expired_conns; while (ctx) { @@ -186,9 +196,9 @@ pxy_thrmgr_print_thr_info(pxy_thr_ctx_t *tctx) } } - if (asprintf(&smsg, "STATS: thr=%d, mld=%zu, mfd=%d, mat=%lld, mct=%lld, iib=%llu, iob=%llu, eib=%llu, eob=%llu, swm=%zu, uwm=%zu, to=%zu, err=%zu, si=%u\n", + if (asprintf(&smsg, "STATS: thr=%d, mld=%zu, mfd=%d, mat=%lld, mct=%lld, iib=%llu, iob=%llu, eib=%llu, eob=%llu, swm=%zu, uwm=%zu, to=%zu, err=%zu, pc=%llu, si=%u\n", tctx->thridx, tctx->max_load, tctx->max_fd, (long long)max_atime, (long long)max_ctime, tctx->intif_in_bytes, tctx->intif_out_bytes, tctx->extif_in_bytes, tctx->extif_out_bytes, - tctx->set_watermarks, tctx->unset_watermarks, tctx->timedout_conns, tctx->errors, tctx->stats_idx) < 0) { + tctx->set_watermarks, tctx->unset_watermarks, tctx->timedout_conns, tctx->errors, tctx->pending_ssl_conn_count, tctx->stats_idx) < 0) { goto leave; } @@ -460,6 +470,67 @@ pxy_thrmgr_free(pxy_thrmgr_ctx_t *ctx) free(ctx); } +void +pxy_thrmgr_add_pending_ssl_conn(pxy_conn_ctx_t *ctx) +{ + pthread_mutex_lock(&ctx->thr->mutex); + if (!ctx->sslctx->pending) { +#ifdef DEBUG_PROXY + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_add_pending_ssl_conn: Adding conn, id=%llu, fd=%d\n", ctx->id, ctx->fd); +#endif /* DEBUG_PROXY */ + + ctx->sslctx->pending = 1; + ctx->thr->pending_ssl_conn_count++; + ctx->sslctx->next_pending = ctx->thr->pending_ssl_conns; + ctx->thr->pending_ssl_conns = ctx; + } + pthread_mutex_unlock(&ctx->thr->mutex); +} + +static void NONNULL(1) +pxy_thrmgr_remove_pending_ssl_conn_unlocked(pxy_conn_ctx_t *ctx) +{ + if (ctx->sslctx && ctx->sslctx->pending) { +#ifdef DEBUG_PROXY + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_remove_pending_ssl_conn_unlocked: Removing conn, id=%llu, fd=%d\n", ctx->id, ctx->fd); +#endif /* DEBUG_PROXY */ + + ctx->sslctx->pending = 0; + ctx->thr->pending_ssl_conn_count--; + + // @attention We may get multiple conns with the same fd combinations, so fds cannot uniquely define a conn; hence the need for unique ids. + if (ctx->id == ctx->thr->pending_ssl_conns->id) { + ctx->thr->pending_ssl_conns = ctx->thr->pending_ssl_conns->sslctx->next_pending; + return; + } else { + pxy_conn_ctx_t *current = ctx->thr->pending_ssl_conns->sslctx->next_pending; + pxy_conn_ctx_t *previous = ctx->thr->pending_ssl_conns; + while (current != NULL && previous != NULL) { + if (ctx->id == current->id) { + previous->sslctx->next_pending = current->sslctx->next_pending; + return; + } + previous = current; + current = current->sslctx->next_pending; + } + // This should never happen + log_err_level_printf(LOG_CRIT, "Cannot find conn in thrmgr pending_conns\n"); +#ifdef DEBUG_PROXY + log_dbg_level_printf(LOG_DBG_MODE_FINE, "pxy_thrmgr_remove_pending_ssl_conn_unlocked: Cannot find conn in thrmgr pending_conns, id=%llu, fd=%d\n", ctx->id, ctx->fd); +#endif /* DEBUG_PROXY */ + assert(0); + } + } +} + +void +pxy_thrmgr_remove_pending_ssl_conn(pxy_conn_ctx_t *ctx) +{ + pthread_mutex_lock(&ctx->thr->mutex); + pxy_thrmgr_remove_pending_ssl_conn_unlocked(ctx); + pthread_mutex_unlock(&ctx->thr->mutex); +} + void pxy_thrmgr_add_conn(pxy_conn_ctx_t *ctx) { @@ -486,7 +557,7 @@ pxy_thrmgr_add_conn(pxy_conn_ctx_t *ctx) } static void NONNULL(1) -pxy_thrmgr_remove_conn(pxy_conn_ctx_t *ctx) +pxy_thrmgr_remove_conn_unlocked(pxy_conn_ctx_t *ctx) { assert(ctx != NULL); assert(ctx->thr->conns != NULL); @@ -494,7 +565,7 @@ pxy_thrmgr_remove_conn(pxy_conn_ctx_t *ctx) if (ctx->in_thr_conns) { #ifdef DEBUG_PROXY - log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_remove_conn: Removing conn, id=%llu, fd=%d\n", ctx->id, ctx->fd); + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_remove_conn_unlocked: Removing conn, id=%llu, fd=%d\n", ctx->id, ctx->fd); #endif /* DEBUG_PROXY */ // We increment thr load in pxy_thrmgr_add_conn() only (for parent conns) @@ -520,14 +591,14 @@ pxy_thrmgr_remove_conn(pxy_conn_ctx_t *ctx) // This should never happen log_err_level_printf(LOG_CRIT, "Cannot find conn in thr conns\n"); #ifdef DEBUG_PROXY - log_dbg_level_printf(LOG_DBG_MODE_FINE, "pxy_thrmgr_remove_conn: Cannot find conn in thr conns, id=%llu, fd=%d\n", ctx->id, ctx->fd); + log_dbg_level_printf(LOG_DBG_MODE_FINE, "pxy_thrmgr_remove_conn_unlocked: Cannot find conn in thr conns, id=%llu, fd=%d\n", ctx->id, ctx->fd); #endif /* DEBUG_PROXY */ assert(0); } } else { // This can happen if we are closing the conn after a fatal error before setting its event callback #ifdef DEBUG_PROXY - log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_remove_conn: Conn not in thr conns, id=%llu, fd=%d\n", ctx->id, ctx->fd); + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_remove_conn_unlocked: Conn not in thr conns, id=%llu, fd=%d\n", ctx->id, ctx->fd); #endif /* DEBUG_PROXY */ } } @@ -605,38 +676,39 @@ pxy_thrmgr_attach_child(pxy_conn_ctx_t *ctx) * This function cannot fail. */ void -pxy_thrmgr_detach(pxy_conn_ctx_t *ctx) +pxy_thrmgr_detach_unlocked(pxy_conn_ctx_t *ctx) { #ifdef DEBUG_PROXY - log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_detach: ENTER\n"); + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_detach_unlocked: ENTER\n"); #endif /* DEBUG_PROXY */ - pxy_thrmgr_remove_conn(ctx); + pxy_thrmgr_remove_pending_ssl_conn_unlocked(ctx); + pxy_thrmgr_remove_conn_unlocked(ctx); } void -pxy_thrmgr_detach_locked(pxy_conn_ctx_t *ctx) +pxy_thrmgr_detach(pxy_conn_ctx_t *ctx) { pthread_mutex_lock(&ctx->thr->mutex); - pxy_thrmgr_detach(ctx); + pxy_thrmgr_detach_unlocked(ctx); pthread_mutex_unlock(&ctx->thr->mutex); } void -pxy_thrmgr_detach_child(pxy_conn_ctx_t *ctx) +pxy_thrmgr_detach_child_unlocked(pxy_conn_ctx_t *ctx) { #ifdef DEBUG_PROXY - log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_detach_child: ENTER\n"); + log_dbg_level_printf(LOG_DBG_MODE_FINEST, "pxy_thrmgr_detach_child_unlocked: ENTER\n"); #endif /* DEBUG_PROXY */ ctx->thr->load--; } void -pxy_thrmgr_detach_child_locked(pxy_conn_ctx_t *ctx) +pxy_thrmgr_detach_child(pxy_conn_ctx_t *ctx) { pthread_mutex_lock(&ctx->thr->mutex); - pxy_thrmgr_detach_child(ctx); + pxy_thrmgr_detach_child_unlocked(ctx); pthread_mutex_unlock(&ctx->thr->mutex); } diff --git a/pxythrmgr.h b/pxythrmgr.h index ab8f0f4..cadf6da 100644 --- a/pxythrmgr.h +++ b/pxythrmgr.h @@ -68,6 +68,8 @@ typedef struct pxy_thr_ctx { pxy_conn_ctx_t *conns; struct sqlite3_stmt *get_user; pthread_mutex_t mutex; + pxy_conn_ctx_t *pending_ssl_conns; + long long unsigned int pending_ssl_conn_count; } pxy_thr_ctx_t; struct pxy_thrmgr_ctx { @@ -81,14 +83,17 @@ pxy_thrmgr_ctx_t * pxy_thrmgr_new(opts_t *) MALLOC; int pxy_thrmgr_run(pxy_thrmgr_ctx_t *) NONNULL(1) WUNRES; void pxy_thrmgr_free(pxy_thrmgr_ctx_t *) NONNULL(1); +void pxy_thrmgr_add_pending_ssl_conn(pxy_conn_ctx_t *) NONNULL(1); +void pxy_thrmgr_remove_pending_ssl_conn(pxy_conn_ctx_t *) NONNULL(1); + 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_unlocked(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_unlocked(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 */