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
Review at https://gerrit.osmocom.org/3825
osmux: fix buffer management mess in snprintf() calls
SNPRINTF_BUFFER_SIZE() macro seems to be very buggy. Replace it by a
working version:
if (ret < 0) \
ret = 0; \
offset += ret; \
if (ret > remain) \
ret = remain; \
remain -= ret;
That basically accounts for remaining space in the buffer and updates
the offset to where we should print next. This macro also deals with two
corner cases:
1) snprintf() fails, actually never happens in practise, but
documentation indicates it may return -1, so catch this case 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().
As in snprintf(), caller should not assume the buffer is nul-terminated.
Thanks to Pau Espin for reporting, and Holger for clues on this.
I also 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, 49 insertions(+), 45 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/25/3825/1
diff --git a/src/osmux.c b/src/osmux.c
index 23a6440..232b29e 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,16 +875,16 @@
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;
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;
}
@@ -891,11 +892,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)) {
@@ -912,10 +914,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);
@@ -928,12 +928,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 63f89f1..fe200fc 100644
--- a/tests/osmux/osmux_test.c
+++ b/tests/osmux/osmux_test.c
@@ -65,8 +65,9 @@
static void tx_cb(struct msgb *msg, void *data)
{
- char buf[4096];
struct rtp_hdr *rtph = (struct rtp_hdr *)msg->data;
+ char buf[4096];
+ int ret;
#if OSMUX_TEST_USE_TIMING
struct timeval now, diff;
@@ -82,7 +83,8 @@
}
#endif
- osmo_rtp_snprintf(buf, sizeof(buf), msg);
+ ret = osmo_rtp_snprintf(buf, sizeof(buf), msg);
+ buf[ret - 1] = '\0';
fprintf(stderr, "extracted packet: %s\n", buf);
if (memcmp(msg->data + sizeof(struct rtp_hdr),
@@ -102,11 +104,13 @@
static void osmux_deliver(struct msgb *batch_msg, void *data)
{
- char buf[1024];
struct osmux_hdr *osmuxh;
LLIST_HEAD(list);
+ char buf[1024];
+ int ret;
- osmux_snprintf(buf, sizeof(buf), batch_msg);
+ ret = osmux_snprintf(buf, sizeof(buf), batch_msg);
+ buf[ret - 1] = '\0';
fprintf(stderr, "OSMUX message (len=%d) %s\n", batch_msg->len, buf);
/* For each OSMUX message, extract the RTP messages and put them
@@ -182,11 +186,11 @@
static void osmux_test_loop(int ccid)
{
+ struct rtp_hdr *rtph = (struct rtp_hdr *)rtp_pkt;
+ int i, j, k = 0, ret;
struct msgb *msg;
char buf[1024];
- struct rtp_hdr *rtph = (struct rtp_hdr *)rtp_pkt;
uint16_t seq;
- int i, j, k = 0;
for (i = 1; i < 65; i++) {
msg = msgb_alloc(1500, "test");
@@ -200,7 +204,8 @@
seq++;
rtph->sequence = htons(seq);
- osmo_rtp_snprintf(buf, sizeof(buf), msg);
+ ret = osmo_rtp_snprintf(buf, sizeof(buf), msg);
+ buf[ret - 1] = '\0';
fprintf(stderr, "adding to ccid=%u %s\n", (i % 2) + ccid, buf);
rtp_pkts++;
--
To view, visit https://gerrit.osmocom.org/3825
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso <pablo at gnumonks.org>