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.

diff --git a/include/haproxy/quic_tls.h b/include/haproxy/quic_tls.h
index c808405a1a93eddec37a12611897767eb35aaa91..1687d3c0615b100e483557dd61dc1dc1e9f7735b 100644 (file)
--- a/include/haproxy/quic_tls.h
+++ b/include/haproxy/quic_tls.h
@@ -654,7 +654,9 @@ static inline int quic_tls_kp_init(struct quic_tls_kp *kp)
 
 /* Initialize all the key update key phase structures for <qc>
  * 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)
 {
@@ -670,7 +672,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 8b09d57809d3a0745fb1a452c12fc7d83fb423db..91d9808b4a7b4ef604e2e3457eff8e9a59556fe4 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -4582,7 +4582,9 @@ static void quic_conn_enc_level_uninit(struct quic_conn *qc, struct quic_enc_lev
 
 /* Initialize QUIC TLS encryption level with <level<> as level for <qc> 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)
@@ -4606,11 +4608,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;
@@ -4623,17 +4625,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. */
@@ -4778,12 +4776,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);
@@ -4879,7 +4890,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;
@@ -4889,9 +4899,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));
@@ -4933,10 +4940,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;
 }
 
@@ -4998,7 +5006,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);