QUIC RX: Support refcounted packets and eliminate wrapper

Previously, the QRX filled in a OSSL_QRX_PKT structure provided by the
caller. This necessitated the caller managing reference counting itself
using a OSSL_QRX_PKT_WRAP structure. The need for this structure has
been eliminated by adding refcounting support to the QRX itself. The QRX
now outputs a pointer to an OSSL_QRX_PKT instead of filling in a
structure provided by the caller. The OSSL_QRX_PKT_WRAP structure has
been eliminated.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19703)

6 files changed, 60 insertions, 117 deletions

diff --git a/ssl/quic/build.info b/ssl/quic/build.info
index 1380112197..0d84df4be3 100644
--- a/ssl/quic/build.info
+++ b/ssl/quic/build.info
@@ -3,10 +3,9 @@ $LIBSSL=../../libssl
 SOURCE[$LIBSSL]=quic_method.c quic_impl.c quic_wire.c quic_ackm.c quic_statm.c
 SOURCE[$LIBSSL]=cc_dummy.c quic_demux.c quic_record_rx.c
 SOURCE[$LIBSSL]=quic_record_tx.c quic_record_util.c quic_record_shared.c quic_wire_pkt.c
-SOURCE[$LIBSSL]=quic_record_rx_wrap.c quic_rx_depack.c
-SOURCE[$LIBSSL]=quic_fc.c uint_set.c quic_sf_list.c quic_rstream.c quic_sstream.c
+SOURCE[$LIBSSL]=quic_rx_depack.c
+SOURCE[$LIBSSL]=quic_fc.c uint_set.c
 SOURCE[$LIBSSL]=quic_cfq.c quic_txpim.c quic_fifd.c quic_txp.c
 SOURCE[$LIBSSL]=quic_stream_map.c
 SOURCE[$LIBSSL]=quic_sf_list.c quic_rstream.c quic_sstream.c
-SOURCE[$LIBSSL]=quic_sf_list.c quic_rstream.c quic_sstream.c
 SOURCE[$LIBSSL]=quic_dummy_handshake.c
diff --git a/ssl/quic/quic_record_rx.c b/ssl/quic/quic_record_rx.c
index 5203e818a6..eb975901ed 100644
--- a/ssl/quic/quic_record_rx.c
+++ b/ssl/quic/quic_record_rx.c
@@ -42,7 +42,8 @@ typedef struct rxe_st RXE;
 
 struct rxe_st {
     OSSL_LIST_MEMBER(rxe, RXE);
-    size_t              data_len, alloc_len;
+    OSSL_QRX_PKT        pkt;
+    size_t              data_len, alloc_len, refcount;
 
     /* Extra fields for per-packet information. */
     QUIC_PKT_HDR        hdr; /* data/len are decrypted payload */
@@ -329,6 +330,7 @@ static RXE *qrx_alloc_rxe(size_t alloc_len)
     ossl_list_rxe_init_elem(rxe);
     rxe->alloc_len = alloc_len;
     rxe->data_len  = 0;
+    rxe->refcount  = 0;
     return rxe;
 }
 
@@ -414,6 +416,9 @@ static void qrx_recycle_rxe(OSSL_QRX *qrx, RXE *rxe)
 {
     /* RXE should not be in any list */
     assert(ossl_list_rxe_prev(rxe) == NULL && ossl_list_rxe_next(rxe) == NULL);
+    rxe->pkt.hdr    = NULL;
+    rxe->pkt.peer   = NULL;
+    rxe->pkt.local  = NULL;
     ossl_list_rxe_insert_tail(&qrx->rx_free, rxe);
 }
 
@@ -1054,7 +1059,7 @@ static int qrx_process_pending_urxl(OSSL_QRX *qrx)
     return 1;
 }
 
-int ossl_qrx_read_pkt(OSSL_QRX *qrx, OSSL_QRX_PKT *pkt)
+int ossl_qrx_read_pkt(OSSL_QRX *qrx, OSSL_QRX_PKT **ppkt)
 {
     RXE *rxe;
 
@@ -1070,25 +1075,41 @@ int ossl_qrx_read_pkt(OSSL_QRX *qrx, OSSL_QRX_PKT *pkt)
     if (!ossl_assert(rxe != NULL))
         return 0;
 
-    pkt->handle         = rxe;
-    pkt->hdr            = &rxe->hdr;
-    pkt->pn             = rxe->pn;
-    pkt->time           = rxe->time;
-    pkt->datagram_len   = rxe->datagram_len;
-    pkt->peer
+    assert(rxe->refcount == 0);
+    rxe->refcount = 1;
+
+    rxe->pkt.hdr            = &rxe->hdr;
+    rxe->pkt.pn             = rxe->pn;
+    rxe->pkt.time           = rxe->time;
+    rxe->pkt.datagram_len   = rxe->datagram_len;
+    rxe->pkt.peer
         = BIO_ADDR_family(&rxe->peer) != AF_UNSPEC ? &rxe->peer : NULL;
-    pkt->local
+    rxe->pkt.local
         = BIO_ADDR_family(&rxe->local) != AF_UNSPEC ? &rxe->local : NULL;
+    rxe->pkt.qrx            = qrx;
+    *ppkt = &rxe->pkt;
     return 1;
 }
 
-void ossl_qrx_release_pkt(OSSL_QRX *qrx, void *handle)
+void ossl_qrx_pkt_release(OSSL_QRX_PKT *pkt)
 {
-    if (handle != NULL) {
-        RXE *rxe = handle;
+    RXE *rxe;
 
-        qrx_recycle_rxe(qrx, rxe);
-    }
+    if (pkt == NULL)
+        return;
+
+    rxe = (RXE *)pkt;
+    assert(rxe->refcount > 0);
+    if (--rxe->refcount == 0)
+        qrx_recycle_rxe(pkt->qrx, rxe);
+}
+
+void ossl_qrx_pkt_up_ref(OSSL_QRX_PKT *pkt)
+{
+    RXE *rxe = (RXE *)pkt;
+
+    assert(rxe->refcount > 0);
+    ++rxe->refcount;
 }
 
 uint64_t ossl_qrx_get_bytes_received(OSSL_QRX *qrx, int clear)
diff --git a/ssl/quic/quic_record_rx_wrap.c b/ssl/quic/quic_record_rx_wrap.c
deleted file mode 100644
index 1c98682377..0000000000
--- a/ssl/quic/quic_record_rx_wrap.c
+++ /dev/null
@@ -1,65 +0,0 @@
-/*
- * Copyright 2022 The OpenSSL Project Authors. All Rights Reserved.
- *
- * Licensed under the Apache License 2.0 (the "License").  You may not use
- * this file except in compliance with the License.  You can obtain a copy
- * in the file LICENSE in the source distribution or at
- * https://www.openssl.org/source/license.html
- */
-
-#include "internal/cryptlib.h"
-#include "internal/refcount.h"
-#include "internal/quic_record_rx_wrap.h"
-
-OSSL_QRX_PKT_WRAP *ossl_qrx_pkt_wrap_new(OSSL_QRX_PKT *pkt)
-{
-    CRYPTO_RWLOCK *refcount_lock = NULL;
-    OSSL_QRX_PKT_WRAP *res = NULL;
-
-    if (pkt == NULL)
-        return NULL;
-
-#ifdef HAVE_ATOMICS
-    refcount_lock = CRYPTO_THREAD_lock_new();
-    if (refcount_lock == NULL) {
-        ERR_raise(ERR_LIB_SSL, ERR_R_CRYPTO_LIB);
-        return NULL;
-    }
-#endif
-
-    if ((res = OPENSSL_zalloc(sizeof(*res))) == NULL) {
-        CRYPTO_THREAD_lock_free(refcount_lock);
-        return NULL;
-    }
-
-    res->pkt = pkt;
-    res->handle = pkt->handle;
-    res->references = 1;
-    res->lock = refcount_lock;
-
-    return res;
-}
-
-int ossl_qrx_pkt_wrap_up_ref(OSSL_QRX_PKT_WRAP *pkt_wrap)
-{
-    int ref = 0;
-
-    if (pkt_wrap == NULL || pkt_wrap->pkt == NULL)
-        return 0;
-    CRYPTO_UP_REF(&pkt_wrap->references, &ref, pkt_wrap->lock);
-    return 1;
-}
-
-void ossl_qrx_pkt_wrap_free(OSSL_QRX *qrx, OSSL_QRX_PKT_WRAP *pkt_wrap)
-{
-    int ref = 0;
-
-    if (pkt_wrap == NULL)
-        return;
-    CRYPTO_DOWN_REF(&pkt_wrap->references, &ref, pkt_wrap->lock);
-    if (ref > 0)
-        return;
-    ossl_qrx_release_pkt(qrx, pkt_wrap->handle);
-    CRYPTO_THREAD_lock_free(pkt_wrap->lock);
-    OPENSSL_free(pkt_wrap);
-}
diff --git a/ssl/quic/quic_rstream.c b/ssl/quic/quic_rstream.c
index 9c4b8b716b..f8ad8b89de 100644
--- a/ssl/quic/quic_rstream.c
+++ b/ssl/quic/quic_rstream.c
@@ -17,7 +17,7 @@ struct quic_rstream_st {
     OSSL_STATM *statm;
 };
 
-QUIC_RSTREAM *ossl_quic_rstream_new(OSSL_QRX *qrx, QUIC_RXFC *rxfc,
+QUIC_RSTREAM *ossl_quic_rstream_new(QUIC_RXFC *rxfc,
                                     OSSL_STATM *statm)
 {
     QUIC_RSTREAM *ret = OPENSSL_malloc(sizeof(*ret));
@@ -25,7 +25,7 @@ QUIC_RSTREAM *ossl_quic_rstream_new(OSSL_QRX *qrx, QUIC_RXFC *rxfc,
     if (ret == NULL)
         return NULL;
 
-    ossl_sframe_list_init(&ret->fl, qrx);
+    ossl_sframe_list_init(&ret->fl);
     ret->rxfc = rxfc;
     ret->statm = statm;
     return ret;
@@ -37,7 +37,7 @@ void ossl_quic_rstream_free(QUIC_RSTREAM *qrs)
     OPENSSL_free(qrs);
 }
 
-int ossl_quic_rstream_queue_data(QUIC_RSTREAM *qrs, OSSL_QRX_PKT_WRAP *pkt_wrap,
+int ossl_quic_rstream_queue_data(QUIC_RSTREAM *qrs, OSSL_QRX_PKT *pkt,
                                  uint64_t offset,
                                  const unsigned char *data, uint64_t data_len,
                                  int fin)
@@ -47,7 +47,7 @@ int ossl_quic_rstream_queue_data(QUIC_RSTREAM *qrs, OSSL_QRX_PKT_WRAP *pkt_wrap,
     range.start = offset;
     range.end = offset + data_len;
 
-    return ossl_sframe_list_insert(&qrs->fl, &range, pkt_wrap, data, fin);
+    return ossl_sframe_list_insert(&qrs->fl, &range, pkt, data, fin);
 }
 
 static int read_internal(QUIC_RSTREAM *qrs, unsigned char *buf, size_t size,
diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c
index 66d2edaaae..2c189f20b7 100644
--- a/ssl/quic/quic_rx_depack.c
+++ b/ssl/quic/quic_rx_depack.c
@@ -13,7 +13,6 @@
 #include "internal/quic_record_rx.h"
 #include "internal/quic_ackm.h"
 #include "internal/quic_rx_depack.h"
-#include "internal/quic_record_rx_wrap.h"
 #include "internal/quic_error.h"
 #include "internal/quic_fc.h"
 #include "internal/sockets.h"
@@ -66,7 +65,7 @@ static int ssl_get_stream_type(QUIC_STREAM *stream)
  * reference count.  When the data is consumed (i.e. as a result of, say,
  * SSL_read()), ossl_qrx_pkt_wrap_free() must be called.
  */
-static int ssl_queue_data(QUIC_STREAM *stream, OSSL_QRX_PKT_WRAP *pkt_wrap,
+static int ssl_queue_data(QUIC_STREAM *stream, OSSL_QRX_PKT *pkt,
                           const unsigned char *data, uint64_t data_len,
                           uint64_t logical_offset, int is_fin)
 {
@@ -80,7 +79,7 @@ static int ssl_queue_data(QUIC_STREAM *stream, OSSL_QRX_PKT_WRAP *pkt_wrap,
         return 0;
 
     return stream->rstream == NULL
-           || ossl_quic_rstream_queue_data(stream->rstream, pkt_wrap,
+           || ossl_quic_rstream_queue_data(stream->rstream, pkt,
                                            logical_offset, data, data_len,
                                            is_fin);
 }
@@ -295,7 +294,7 @@ static int depack_do_frame_new_token(PACKET *pkt, QUIC_CONNECTION *connection,
 }
 
 static int depack_do_frame_stream(PACKET *pkt, QUIC_CONNECTION *connection,
-                                  OSSL_QRX_PKT_WRAP *parent_pkt,
+                                  OSSL_QRX_PKT *parent_pkt,
                                   OSSL_ACKM_RX_PKT *ackm_data)
 {
     OSSL_QUIC_FRAME_STREAM frame_data;
@@ -547,10 +546,10 @@ static int depack_do_frame_unknown_extension(PACKET *pkt,
 /* Main frame processor */
 
 static int depack_process_frames(QUIC_CONNECTION *connection, PACKET *pkt,
-                                 OSSL_QRX_PKT_WRAP *parent_pkt, int packet_space,
+                                 OSSL_QRX_PKT *parent_pkt, int packet_space,
                                  OSSL_TIME received, OSSL_ACKM_RX_PKT *ackm_data)
 {
-    uint32_t pkt_type = parent_pkt->pkt->hdr->type;
+    uint32_t pkt_type = parent_pkt->hdr->type;
 
     while (PACKET_remaining(pkt) > 0) {
         uint64_t frame_type;
@@ -748,7 +747,6 @@ int ossl_quic_handle_frames(QUIC_CONNECTION *connection, OSSL_QRX_PKT *qpacket)
 {
     PACKET pkt;
     OSSL_ACKM_RX_PKT ackm_data;
-    OSSL_QRX_PKT_WRAP *qpkt_wrap = NULL;
     /*
      * ok has three states:
      * -1 error with ackm_data uninitialized
@@ -760,9 +758,6 @@ int ossl_quic_handle_frames(QUIC_CONNECTION *connection, OSSL_QRX_PKT *qpacket)
     if (connection == NULL)
         goto end;
 
-    if ((qpkt_wrap = ossl_qrx_pkt_wrap_new(qpacket)) == NULL)
-        goto end;
-
     /* Initialize |ackm_data| (and reinitialize |ok|)*/
     memset(&ackm_data, 0, sizeof(ackm_data));
     /*
@@ -796,7 +791,7 @@ int ossl_quic_handle_frames(QUIC_CONNECTION *connection, OSSL_QRX_PKT *qpacket)
 
     /* Now that special cases are out of the way, parse frames */
     if (!PACKET_buf_init(&pkt, qpacket->hdr->data, qpacket->hdr->len)
-        || !depack_process_frames(connection, &pkt, qpkt_wrap,
+        || !depack_process_frames(connection, &pkt, qpacket,
                                   ackm_data.pkt_space, qpacket->time,
                                   &ackm_data))
         goto end;
@@ -814,19 +809,16 @@ int ossl_quic_handle_frames(QUIC_CONNECTION *connection, OSSL_QRX_PKT *qpacket)
         ossl_ackm_on_rx_packet(GET_CONN_ACKM(connection), &ackm_data);
 
     /*
-     * Let go of the packet pointer in |qpkt_wrap|.  This means that the
-     * reference counter can't be incremented any more.
+     * Release the ref to the packet. This will free the packet unless something
+     * in our processing above has added a reference to it.
      */
-    if (qpkt_wrap != NULL)
-        qpkt_wrap->pkt = NULL;
-
-    ossl_qrx_pkt_wrap_free(GET_CONN_QRX(connection), qpkt_wrap);
+    ossl_qrx_pkt_release(qpacket);
     return ok > 0;
 }
 
 int ossl_quic_depacketize(QUIC_CONNECTION *connection)
 {
-    OSSL_QRX_PKT qpacket;
+    OSSL_QRX_PKT *qpacket = NULL;
 
     if (connection == NULL)
         return 0;
@@ -836,5 +828,5 @@ int ossl_quic_depacketize(QUIC_CONNECTION *connection)
     if (ossl_qrx_read_pkt(GET_CONN_QRX(connection), &qpacket) <= 0)
         return 0;
 
-    return ossl_quic_handle_frames(connection, &qpacket);
+    return ossl_quic_handle_frames(connection, qpacket);
 }
diff --git a/ssl/quic/quic_sf_list.c b/ssl/quic/quic_sf_list.c
index d382bd0904..2924b78e1f 100644
--- a/ssl/quic/quic_sf_list.c
+++ b/ssl/quic/quic_sf_list.c
@@ -9,31 +9,28 @@
 
 #include "internal/uint_set.h"
 #include "internal/common.h"
-#include "internal/quic_record_rx_wrap.h"
 #include "internal/quic_sf_list.h"
 
 struct stream_frame_st {
     struct stream_frame_st *prev, *next;
     UINT_RANGE range;
-    OSSL_QRX_PKT_WRAP *pkt;
+    OSSL_QRX_PKT *pkt;
     const unsigned char *data;
 };
 
 static void stream_frame_free(SFRAME_LIST *fl, STREAM_FRAME *sf)
 {
-    ossl_qrx_pkt_wrap_free(fl->qrx, sf->pkt);
+    ossl_qrx_pkt_release(sf->pkt);
     OPENSSL_free(sf);
 }
 
-static STREAM_FRAME *stream_frame_new(UINT_RANGE *range, OSSL_QRX_PKT_WRAP *pkt,
+static STREAM_FRAME *stream_frame_new(UINT_RANGE *range, OSSL_QRX_PKT *pkt,
                                       const unsigned char *data)
 {
     STREAM_FRAME *sf = OPENSSL_zalloc(sizeof(*sf));
 
-    if (pkt != NULL && !ossl_qrx_pkt_wrap_up_ref(pkt)) {
-        OPENSSL_free(sf);
-        return NULL;
-    }
+    if (pkt != NULL)
+        ossl_qrx_pkt_up_ref(pkt);
 
     sf->range = *range;
     sf->pkt = pkt;
@@ -42,10 +39,9 @@ static STREAM_FRAME *stream_frame_new(UINT_RANGE *range, OSSL_QRX_PKT_WRAP *pkt,
     return sf;
 }
 
-void ossl_sframe_list_init(SFRAME_LIST *fl, OSSL_QRX *qrx)
+void ossl_sframe_list_init(SFRAME_LIST *fl)
 {
     memset(fl, 0, sizeof(*fl));
-    fl->qrx = qrx;
 }
 
 void ossl_sframe_list_destroy(SFRAME_LIST *fl)
@@ -59,7 +55,7 @@ void ossl_sframe_list_destroy(SFRAME_LIST *fl)
 }
 
 static int append_frame(SFRAME_LIST *fl, UINT_RANGE *range,
-                        OSSL_QRX_PKT_WRAP *pkt,
+                        OSSL_QRX_PKT *pkt,
                         const unsigned char *data)
 {
     STREAM_FRAME *new_frame;
@@ -75,7 +71,7 @@ static int append_frame(SFRAME_LIST *fl, UINT_RANGE *range,
 }
 
 int ossl_sframe_list_insert(SFRAME_LIST *fl, UINT_RANGE *range,
-                            OSSL_QRX_PKT_WRAP *pkt,
+                            OSSL_QRX_PKT *pkt,
                             const unsigned char *data, int fin)
 {
     STREAM_FRAME *sf, *new_frame, *prev_frame, *next_frame;