From e78bb0f6b1769c25228e93a357e421efcc735c93 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 12 Dec 2022 11:22:42 +0100 Subject: [PATCH] BUG/MINOR: quic: properly handle alloc failure in qc_new_conn() qc_new_conn() is used to allocate a quic_conn instance and its various internal members. If one allocation fails, quic_conn_release() is used to cleanup things. For the moment, pool_zalloc() is used which ensures that all content is null. However, some members must be initialized to a special values to be able to use quic_conn_release() safely. This is the case for quic_conn lists and its tasklet. Also, some quic_conn internal allocation functions were doing their own cleanup on failure without reset to NULL. This caused an issue with quic_conn_release() which also frees this members. To fix this, these functions now only return an error without cleanup. It is the caller responsibility to free the allocated content, which is done via quic_conn_release(). Without this patch, allocation failure in qc_new_conn() would often result in segfault. This was reproduced easily using fail-alloc at 10%. This should be backported up to 2.6. (cherry picked from commit dbf6ad470b3206f64254141e7cf80a980261be29) Signed-off-by: Christopher Faulet (cherry picked from commit d35d46916d8ff53b13c08862297f49b5d881d738) Signed-off-by: Christopher Faulet --- include/haproxy/quic_tls.h | 5 +++-- src/quic_conn.c | 41 +++++++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/include/haproxy/quic_tls.h b/include/haproxy/quic_tls.h index 115d0fe54..142906794 100644 --- a/include/haproxy/quic_tls.h +++ b/include/haproxy/quic_tls.h @@ -652,7 +652,9 @@ static inline int quic_tls_kp_init(struct quic_tls_kp *kp) /* Initialize all the key update key phase structures for * QUIC connection, allocating the required memory. - * Returns 1 if succeeded, 0 if not. + * + * Returns 1 if succeeded, 0 if not. The caller is responsible to use + * quic_tls_ku_free() on error to cleanup partially allocated content. */ static inline int quic_tls_ku_init(struct quic_conn *qc) { @@ -668,7 +670,6 @@ static inline int quic_tls_ku_init(struct quic_conn *qc) return 1; err: - quic_tls_ku_free(qc); return 0; } diff --git a/src/quic_conn.c b/src/quic_conn.c index a8edc431b..8cefe1b65 100644 --- a/src/quic_conn.c +++ b/src/quic_conn.c @@ -4566,7 +4566,9 @@ static void quic_conn_enc_level_uninit(struct quic_conn *qc, struct quic_enc_lev /* Initialize QUIC TLS encryption level with as level for QUIC * connection allocating everything needed. - * Returns 1 if succeeded, 0 if not. + * + * Returns 1 if succeeded, 0 if not. On error the caller is responsible to use + * quic_conn_enc_level_uninit() to cleanup partially allocated content. */ static int quic_conn_enc_level_init(struct quic_conn *qc, enum quic_tls_enc_level level) @@ -4590,11 +4592,11 @@ static int quic_conn_enc_level_init(struct quic_conn *qc, /* TODO: use a pool */ qel->tx.crypto.bufs = malloc(sizeof *qel->tx.crypto.bufs); if (!qel->tx.crypto.bufs) - goto err; + goto leave; qel->tx.crypto.bufs[0] = pool_alloc(pool_head_quic_crypto_buf); if (!qel->tx.crypto.bufs[0]) - goto err; + goto leave; qel->tx.crypto.bufs[0]->sz = 0; qel->tx.crypto.nb_buf = 1; @@ -4607,17 +4609,13 @@ static int quic_conn_enc_level_init(struct quic_conn *qc, else { qel->cstream = quic_cstream_new(qc); if (!qel->cstream) - goto err; + goto leave; } ret = 1; leave: TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc); return ret; - - err: - ha_free(&qel->tx.crypto.bufs); - goto leave; } /* Callback called upon loss detection and PTO timer expirations. */ @@ -4762,12 +4760,25 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, struct quic_cc_algo *cc_algo = NULL; struct quic_tls_ctx *ictx; TRACE_ENTER(QUIC_EV_CONN_INIT); + /* TODO replace pool_zalloc by pool_alloc(). This requires special care + * to properly initialized internal quic_conn members to safely use + * quic_conn_release() on alloc failure. + */ qc = pool_zalloc(pool_head_quic_conn); if (!qc) { TRACE_ERROR("Could not allocate a new connection", QUIC_EV_CONN_INIT); goto err; } + /* Initialize in priority qc members required for a safe dealloc. */ + + /* required to use MTLIST_IN_LIST */ + MT_LIST_INIT(&qc->accept_list); + + LIST_INIT(&qc->rx.pkt_list); + + /* Now proceeds to allocation of qc members. */ + buf_area = pool_alloc(pool_head_quic_conn_rxbuf); if (!buf_area) { TRACE_ERROR("Could not allocate a new RX buffer", QUIC_EV_CONN_INIT, qc); @@ -4854,7 +4865,6 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, qc->nb_pkt_for_cc = 1; qc->nb_pkt_since_cc = 0; - LIST_INIT(&qc->rx.pkt_list); if (!quic_tls_ku_init(qc)) { TRACE_ERROR("Key update initialization failed", QUIC_EV_CONN_INIT, qc); goto err; @@ -4864,9 +4874,6 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, qc->path = &qc->paths[0]; quic_path_init(qc->path, ipv4, cc_algo ? cc_algo : default_quic_cc_algo, qc); - /* required to use MTLIST_IN_LIST */ - MT_LIST_INIT(&qc->accept_list); - qc->streams_by_id = EB_ROOT_UNIQUE; qc->stream_buf_count = 0; memcpy(&qc->local_addr, local_addr, sizeof(qc->local_addr)); @@ -4907,10 +4914,11 @@ static struct quic_conn *qc_new_conn(const struct quic_version *qv, int ipv4, err: pool_free(pool_head_quic_conn_rxbuf, buf_area); - if (qc) + if (qc) { qc->rx.buf.area = NULL; - quic_conn_release(qc); - TRACE_LEAVE(QUIC_EV_CONN_INIT, qc); + quic_conn_release(qc); + } + TRACE_LEAVE(QUIC_EV_CONN_INIT); return NULL; } @@ -4969,7 +4977,8 @@ void quic_conn_release(struct quic_conn *qc) qc->timer_task = NULL; } - tasklet_free(qc->wait_event.tasklet); + if (qc->wait_event.tasklet) + tasklet_free(qc->wait_event.tasklet); /* remove the connection from receiver cids trees */ ebmb_delete(&qc->odcid_node);