[MERGED] libosmo-netif[master]: osmux: fix buffer management mess in snprintf() calls

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Pablo Neira Ayuso gerrit-no-reply at lists.osmocom.org
Mon Sep 11 18:17:51 UTC 2017


Pablo Neira Ayuso has submitted this change and it was merged.

Change subject: osmux: fix buffer management mess in snprintf() calls
......................................................................


osmux: fix buffer management mess in snprintf() calls

SNPRINTF_BUFFER_SIZE() looks too complex, previous version maintains two
different variables to account for the remaining space in the buffer,
one of them is always decremented based on what snprintf() returns,
which may result in underflow. These variables are swapped - not used
consistently - all over this code.

Replace this macro by a simplified version, with one single parameter to
account for remaining space. This macro also deals with two corner
cases:

1) snprintf() fails, actually never happens in practise, but
   documentation indicates it may return -1, so let's catch this case
   from here to stick to specs.

2) There is not enough space in the buffer, in that case, keep
   increasing offset, so we know how much would have been printed, just
   like snprintf() does.

Thanks to Pau Espin for reporting, and Holger for clues on this.
I have run osmux_test and, at quick glance, it looks good.

Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
---
M src/osmux.c
M src/rtp.c
M tests/osmux/osmux_test.c
3 files changed, 44 insertions(+), 45 deletions(-)

Approvals:
  Pau Espin Pedrol: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/osmux.c b/src/osmux.c
index b3c43e2..b18246f 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -846,19 +846,20 @@
 	h->rtp_ssrc = rtp_ssrc;
 }
 
-#define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)	\
-	size -= ret;					\
-	if (ret > len)					\
-		ret = len;				\
+#define SNPRINTF_BUFFER_SIZE(ret, remain, offset)	\
+	if (ret < 0)					\
+		ret = 0;				\
 	offset += ret;					\
-	len -= ret;
+	if (ret > remain)				\
+		ret = remain;				\
+	remain -= ret;
 
 static int osmux_snprintf_header(char *buf, size_t size, struct osmux_hdr *osmuxh)
 {
+	unsigned int remain = size, offset = 0;
 	int ret;
-	int len = size, offset = 0;
 
-	ret = snprintf(buf, len, "OSMUX seq=%03u ccid=%03u "
+	ret = snprintf(buf, remain, "OSMUX seq=%03u ccid=%03u "
 				 "ft=%01u ctr=%01u "
 				 "amr_f=%01u amr_q=%01u "
 				 "amr_ft=%02u amr_cmr=%02u ",
@@ -866,7 +867,7 @@
 			osmuxh->ft, osmuxh->ctr,
 			osmuxh->amr_f, osmuxh->amr_q,
 			osmuxh->amr_ft, osmuxh->amr_cmr);
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	return offset;
 }
@@ -874,19 +875,19 @@
 static int osmux_snprintf_payload(char *buf, size_t size,
 				  const uint8_t *payload, int payload_len)
 {
+	unsigned int remain = size, offset = 0;
 	int ret, i;
-	int len = size, offset = 0;
 
-	ret = snprintf(buf+offset, len, "[ ");
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	ret = snprintf(buf + offset, remain, "[ ");
+	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	for (i=0; i<payload_len; i++) {
-		ret = snprintf(buf+offset, len, "%02x ", payload[i]);
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		ret = snprintf(buf + offset, remain, "%02x ", payload[i]);
+		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 	}
 
-	ret = snprintf(buf+offset, len, "] ");
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	ret = snprintf(buf + offset, remain, "]");
+	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	return offset;
 }
@@ -894,11 +895,12 @@
 
 int osmux_snprintf(char *buf, size_t size, struct msgb *msg)
 {
-	int ret;
-	unsigned int offset = 0;
-	int msg_len = msg->len, len = size;
-	struct osmux_hdr *osmuxh;
+	unsigned int remain = size;
 	int this_len, msg_off = 0;
+	struct osmux_hdr *osmuxh;
+	unsigned int offset = 0;
+	int msg_len = msg->len;
+	int ret;
 
 	while (msg_len > 0) {
 		if (msg_len < sizeof(struct osmux_hdr)) {
@@ -915,10 +917,8 @@
 			return -1;
 		}
 
-		ret = osmux_snprintf_header(buf+offset, size, osmuxh);
-		if (ret < 0)
-			break;
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		ret = osmux_snprintf_header(buf + offset, remain, osmuxh);
+		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 		this_len = sizeof(struct osmux_hdr) +
 			   osmux_get_payload_len(osmuxh);
@@ -931,12 +931,10 @@
 			return -1;
 		}
 
-		ret = osmux_snprintf_payload(buf+offset, size,
+		ret = osmux_snprintf_payload(buf + offset, remain,
 					     osmux_get_payload(osmuxh),
 					     osmux_get_payload_len(osmuxh));
-		if (ret < 0)
-			break;
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 		msg_len -= this_len;
 	}
diff --git a/src/rtp.c b/src/rtp.c
index 44fc217..56fc37c 100644
--- a/src/rtp.c
+++ b/src/rtp.c
@@ -185,19 +185,20 @@
 	return msg;
 }
 
-#define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)	\
-	size += ret;					\
-	if (ret > len)					\
-		ret = len;				\
+#define SNPRINTF_BUFFER_SIZE(ret, remain, offset)	\
+	if (ret < 0)					\
+		ret = 0;				\
 	offset += ret;					\
-	len -= ret;
+	if (ret > remain)				\
+		ret = remain;				\
+	remain -= ret;
 
 int osmo_rtp_snprintf(char *buf, size_t size, struct msgb *msg)
 {
+	unsigned int remain = size, offset = 0;
 	struct rtp_hdr *rtph;
-	int ret, i;
 	uint8_t *payload;
-	unsigned int len = size, offset = 0;
+	int ret, i;
 
 	rtph = osmo_rtp_get_hdr(msg);
 	if (rtph == NULL)
@@ -205,22 +206,22 @@
 
 	payload = (uint8_t *)rtph + sizeof(struct rtp_hdr);
 
-	ret = snprintf(buf, len, "RTP ver=%01u ssrc=%u type=%02u "
+	ret = snprintf(buf, remain, "RTP ver=%01u ssrc=%u type=%02u "
 			"marker=%01u ext=%01u csrc_count=%01u "
 			"sequence=%u timestamp=%u [", rtph->version,
 			ntohl(rtph->ssrc), rtph->payload_type,
 			rtph->marker, rtph->extension,
 			rtph->csrc_count, ntohs(rtph->sequence),
 			ntohl(rtph->timestamp));
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	for (i=0; i<msg->len - sizeof(struct rtp_hdr); i++) {
-		ret = snprintf(buf+offset, len, "%02x ", payload[i]);
-		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		ret = snprintf(buf + offset, remain, "%02x ", payload[i]);
+		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 	}
 
-	ret = snprintf(buf+offset, len, "]");
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	ret = snprintf(buf + offset, remain, "]");
+	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	return offset;
 }
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c
index bf6174b..09b7a9a 100644
--- a/tests/osmux/osmux_test.c
+++ b/tests/osmux/osmux_test.c
@@ -65,8 +65,8 @@
 
 static void tx_cb(struct msgb *msg, void *data)
 {
-	char buf[4096];
 	struct rtp_hdr *rtph = (struct rtp_hdr *)msg->data;
+	char buf[4096];
 #if OSMUX_TEST_USE_TIMING
 	struct timeval now, diff;
 
@@ -102,9 +102,9 @@
 
 static void osmux_deliver(struct msgb *batch_msg, void *data)
 {
-	char buf[2048];
 	struct osmux_hdr *osmuxh;
 	LLIST_HEAD(list);
+	char buf[2048];
 
 	osmux_snprintf(buf, sizeof(buf), batch_msg);
 	fprintf(stderr, "OSMUX message (len=%d) %s\n", batch_msg->len, buf);
@@ -182,11 +182,11 @@
 
 static void osmux_test_loop(int ccid)
 {
-	struct msgb *msg;
-	char buf[1024];
 	struct rtp_hdr *rtph = (struct rtp_hdr *)rtp_pkt;
-	uint16_t seq;
+	struct msgb *msg;
 	int i, j, k = 0;
+	char buf[1024];
+	uint16_t seq;
 
 	for (i = 1; i < 65; i++) {
 		msg = msgb_alloc(1500, "test");

-- 
To view, visit https://gerrit.osmocom.org/3825
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 4
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>



More information about the gerrit-log mailing list