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.orgHello Pau Espin Pedrol, Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/3825
to look at the new patch set (#2).
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, 44 insertions(+), 45 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/25/3825/2
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: newpatchset
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 2
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: Pau Espin Pedrol <pespin at sysmocom.de>