From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Thu, 30 Jan 2025 15:24:10 +0000 (+0100)
Subject: BUILD: quic: remove GCC undefined error in qc_release_lost_pkts()
X-Git-Tag: v3.2-dev5~43
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4ad2accfee59acad4fd5bc0e2b15b4aa23bd57a1;p=thirdparty%2Fhaproxy.git

BUILD: quic: remove GCC undefined error in qc_release_lost_pkts()

Every once in a while, GCC reports issues with qc_release_lost_pkts()
function. It seems that its static analysis is foiled by the code
structuring. The latest warning reports the following issue :

  CC      src/quic_loss.o
src/quic_loss.c: In function ‘qc_release_lost_pkts’:
src/quic_loss.c:313:58: error: potential null pointer dereference [-Werror=null-dereference]
  313 |                         unsigned int period = newest_lost->time_sent_ms - oldest_lost->time_sent_ms;
      |                                               ~~~~~~~~~~~^~~~~~~~~~~~~~

To fix definitely this, change slightly the code. <oldest_lost> and
<newest_lost> are now initialized on the first list entry outside of the
loop. This is enough to guarantee to GCC that they cannot be NULL for
the remainder of the function.
---

diff --git a/src/quic_loss.c b/src/quic_loss.c
index 79b5d35b7..87c888a22 100644
--- a/src/quic_loss.c
+++ b/src/quic_loss.c
@@ -259,7 +259,11 @@ int qc_release_lost_pkts(struct quic_conn *qc, struct quic_pktns *pktns,
 	if (LIST_ISEMPTY(pkts))
 		goto leave;
 
-	oldest_lost = newest_lost = NULL;
+	/* Oldest will point to first list entry and newest on the last. First,
+	 * initialize them to point on the same entry. Newest pointer will be
+	 * updated along the loop. Release all other packet in between.
+	 */
+	newest_lost = oldest_lost = LIST_ELEM(pkts->n, struct quic_tx_packet *, list);
 	list_for_each_entry_safe(pkt, tmp, pkts, list) {
 		struct list tmp = LIST_HEAD_INIT(tmp);
 
@@ -272,37 +276,28 @@ int qc_release_lost_pkts(struct quic_conn *qc, struct quic_pktns *pktns,
 		if (!qc_handle_frms_of_lost_pkt(qc, pkt, &pktns->tx.frms))
 			close = 1;
 		LIST_DELETE(&pkt->list);
-		if (!oldest_lost) {
-			oldest_lost = newest_lost = pkt;
-		}
-		else {
-			if (newest_lost != oldest_lost)
-				quic_tx_packet_refdec(newest_lost);
-			newest_lost = pkt;
-		}
+
+		/* Move newest so that it will point on the last list entry.
+		 * Release every intermediary packet.
+		 */
+		if (oldest_lost != newest_lost)
+			quic_tx_packet_refdec(newest_lost);
+		newest_lost = pkt;
 		tot_lost++;
 	}
 
-	/* <oldest_lost> cannot be NULL at this stage because we have ensured
-	 * that <pkts> list is not empty. Without this, GCC 12.2.0 reports a
-	 * possible overflow on a 0 byte region with O2 optimization.
-	 */
-	ASSUME_NONNULL(oldest_lost);
-
 	if (!close) {
-		if (newest_lost) {
-			struct quic_cc *cc = &qc->path->cc;
-			/* Sent a congestion event to the controller */
-			struct quic_cc_event ev = { };
+		struct quic_cc *cc = &qc->path->cc;
+		/* Sent a congestion event to the controller */
+		struct quic_cc_event ev = { };
 
-			ev.type = QUIC_CC_EVT_LOSS;
-			ev.loss.time_sent = newest_lost->time_sent_ms;
-			ev.loss.count = tot_lost;
+		ev.type = QUIC_CC_EVT_LOSS;
+		ev.loss.time_sent = newest_lost->time_sent_ms;
+		ev.loss.count = tot_lost;
 
-			quic_cc_event(cc, &ev);
-			if (cc->algo->congestion_event)
-			    cc->algo->congestion_event(cc, newest_lost->time_sent_ms);
-		}
+		quic_cc_event(cc, &ev);
+		if (cc->algo->congestion_event)
+		    cc->algo->congestion_event(cc, newest_lost->time_sent_ms);
 
 		/* If an RTT have been already sampled, <rtt_min> has been set.
 		 * We must check if we are experiencing a persistent congestion.