osmocom
/
libosmo-netif
mirror of https://gerrit.osmocom.org/libosmo-netif
11
0
Fork 0
Code Issues Releases Wiki Activity

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
This commit is contained in:
Pablo Neira Ayuso 2017-09-04 20:35:36 +02:00 committed by Harald Welte
parent 9c5f01e7b2
commit 14af167a55
3 changed files with 44 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
src/osmux.c
View File

@ -846,19 +846,20 @@ void osmux_xfrm_output_init(struct osmux_out_handle *h, uint32_t rtp_ssrc)
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 @@ static int osmux_snprintf_header(char *buf, size_t size, struct osmux_hdr *osmux
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_header(char *buf, size_t size, struct osmux_hdr *osmux
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 @@ static int osmux_snprintf_payload(char *buf, size_t size,
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 @@ int osmux_snprintf(char *buf, size_t size, struct msgb *msg)
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 @@ int osmux_snprintf(char *buf, size_t size, struct msgb *msg)
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;
}

27
src/rtp.c
View File

@ -185,19 +185,20 @@ osmo_rtp_build(struct osmo_rtp_handle *h, uint8_t payload_type,
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 @@ int osmo_rtp_snprintf(char *buf, size_t size, struct msgb *msg)
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;
}

10
tests/osmux/osmux_test.c
View File

@ -65,8 +65,8 @@ static struct timeval last;
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 struct osmux_out_handle h_output;
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_marker(int ccid) {
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");