From: Eugene Syromiatnikov Date: Sun, 12 Apr 2026 12:33:08 +0000 (+0200) Subject: ssl/quic/quic_record_tx.c: refactor qtx->cons obtaining X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=f33bb236fc44afe40a8b4787e14913f82d501dca;p=thirdparty%2Fopenssl.git ssl/quic/quic_record_tx.c: refactor qtx->cons obtaining As currently implemented, the only txe passed to qtx_reserve_txe() (and, subsequently, to qtx_resize_txe()) is qtx->cons one, so the check "if (qtx->cons == txe)" is superfluous, and, more so, would lead to a memory leak if it weren't the case, as was spotted by Coverity. Moreover, the set of qtx_alloc_txe(), qtx_ensure_free_txe(), qtx_ensure_cons(), qtx_resize_txe(), and qtx_reserve_txe() functions, while being written in a relatively generic way, is actually called from a single call site in ossl_qtx_write_pkt(), and contains several duplicating checks and unnecessary logic (like, adding a newly allocated TXE to the free list, only to remove it from there right away in qtx_ensure_cons(), the only its user), so just merge the whole aforementioned set of functions (except qtx_alloc_txe()) in a single function, qtx_get_cons_txe(). Resolves: https://scan5.scan.coverity.com/#/project-view/63999/10222?selectedIssue=1691460 Complements: 16892155e153 "quic: fix NULL txl dereference in qtx_resize_txe" Signed-off-by: Eugene Syromiatnikov Reviewed-by: Saša Nedvědický Reviewed-by: Nikola Pajkovsky MergeDate: Wed Apr 29 15:12:14 2026 (Merged from https://github.com/openssl/openssl/pull/30783) --- diff --git a/ssl/quic/quic_record_tx.c b/ssl/quic/quic_record_tx.c index 0dbdb1491af..f5bd5b2f83c 100644 --- a/ssl/quic/quic_record_tx.c +++ b/ssl/quic/quic_record_tx.c @@ -242,77 +242,49 @@ static TXE *qtx_alloc_txe(size_t alloc_len) } /* - * Ensures there is at least one TXE in the free list, allocating a new entry - * if necessary. The returned TXE is in the free list; it is not popped. - * - * alloc_len is a hint which may be used to determine the TXE size if allocation - * is necessary. Returns NULL on allocation failure. - */ -static TXE *qtx_ensure_free_txe(OSSL_QTX *qtx, size_t alloc_len) -{ - TXE *txe; - - txe = ossl_list_txe_head(&qtx->free); - if (txe != NULL) - return txe; - - txe = qtx_alloc_txe(alloc_len); - if (txe == NULL) - return NULL; - - ossl_list_txe_insert_tail(&qtx->free, txe); - return txe; -} - -/* - * Resize the data buffer attached to an TXE to be n bytes in size. The address - * of the TXE might change; the new address is returned, or NULL on failure, in - * which case the original TXE remains valid. + * Ensure that qtx->cons has a TXE attached with allocated size of at least + * min_size. Returns pointer to the TXE on success or NULL on failure. */ -static TXE *qtx_resize_txe(OSSL_QTX *qtx, TXE *txe, size_t n) +static TXE *qtx_get_cons_txe(OSSL_QTX *qtx, size_t min_size) { - TXE *txe2; - - /* Should never happen. */ - if (txe == NULL) - return NULL; - - if (n >= SIZE_MAX - sizeof(TXE)) + if (min_size >= SIZE_MAX - sizeof(TXE)) return NULL; /* - * to resize txe, the caller must detach it from the list first, - * fail if txe is still attached. + * If there is no coalescing in progress, try to get a TXE + * from the free list (and remove it from there), or allocate a new one. */ - if (!ossl_assert(ossl_list_txe_prev(txe) == NULL - && ossl_list_txe_next(txe) == NULL)) - return NULL; + if (qtx->cons == NULL) { + TXE *txe = ossl_list_txe_head(&qtx->free); - /* - * NOTE: We do not clear old memory, although it does contain decrypted - * data. - */ - txe2 = OPENSSL_realloc(txe, sizeof(TXE) + n); - if (txe2 == NULL) - return NULL; + if (txe != NULL) { + ossl_list_txe_remove(&qtx->free, txe); + } else { + if ((txe = qtx_alloc_txe(min_size)) == NULL) + return NULL; + } - if (qtx->cons == txe) - qtx->cons = txe2; + txe->data_len = 0; + qtx->cons = txe; + qtx->cons_count = 0; + } - txe2->alloc_len = n; - return txe2; -} + /* Resize TXE if it's too small. */ + if (qtx->cons->alloc_len < min_size) { + /* + * NOTE: We do not clear old memory, although it does contain decrypted + * data. + */ + TXE *realloc_txe = OPENSSL_realloc(qtx->cons, sizeof(TXE) + min_size); -/* - * Ensure the data buffer attached to an TXE is at least n bytes in size. - * Returns NULL on failure. - */ -static TXE *qtx_reserve_txe(OSSL_QTX *qtx, TXE *txe, size_t n) -{ - if (txe->alloc_len >= n) - return txe; + if (realloc_txe == NULL) + return NULL; + + realloc_txe->alloc_len = min_size; + qtx->cons = realloc_txe; + } - return qtx_resize_txe(qtx, txe, n); + return qtx->cons; } /* Move a TXE from pending to free. */ @@ -730,24 +702,6 @@ err: return ret; } -static TXE *qtx_ensure_cons(OSSL_QTX *qtx) -{ - TXE *txe = qtx->cons; - - if (txe != NULL) - return txe; - - txe = qtx_ensure_free_txe(qtx, qtx->mdpl); - if (txe == NULL) - return NULL; - - ossl_list_txe_remove(&qtx->free, txe); - qtx->cons = txe; - qtx->cons_count = 0; - txe->data_len = 0; - return txe; -} - static QLOG *qtx_get_qlog(OSSL_QTX *qtx) { if (qtx->get_qlog_cb == NULL) @@ -840,22 +794,14 @@ int ossl_qtx_write_pkt(OSSL_QTX *qtx, const OSSL_QTX_PKT *pkt) * - detached. * * if txe is detached (not member of free/tx list), then it is kept - * in qtx->cons. The qtx_ensure_cons() here either returns the txe - * from free list or existing ->cons txe. The txe we obtain here - * is detached. - */ - txe = qtx_ensure_cons(qtx); - if (txe == NULL) - return 0; /* allocation failure */ - - /* - * Ensure TXE has at least MDPL bytes allocated. This should only be - * possible if the MDPL has increased. + * in qtx->cons. The qtx_get_cons_txe() here makes sure that qtx->cons + * points at a (new or existing) detached txe and has at least MDPL + * bytes allocated. */ - txe = qtx_reserve_txe(qtx, txe, qtx->mdpl); + txe = qtx_get_cons_txe(qtx, qtx->mdpl); if (txe == NULL) { /* - * realloc of txe failed. however it is still kept in ->cons, + * If realloc of txe has failed. it is still kept in ->cons, * no memory leak. * The question is what we should do here to handle error, * is doing `return 0` enough? or shall we discard ->cons and