[PATCH 5/8] mgcp: NUL-terminate MGCP message

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/OpenBSC@lists.osmocom.org/.

Jacob Erlbeck jerlbeck at sysmocom.de
Mon Dec 2 09:21:58 UTC 2013


On 11/30/2013 07:16 PM, Holger Hans Peter Freyther wrote:
> On Fri, Nov 29, 2013 at 01:43:47PM +0100, Jacob Erlbeck wrote:
>> The MGCP message isn't always NUL-terminated when arriving at
>> mgcp_handle_message(). This may lead to undefined results.
> 
> oh!
> 
>> +	/* Ensure that the msg->l2h is NULL terminated. */
>> +	if (msgb_tailroom(msg) > 0)
>> +		*msg->tail = '\0';
>> +	else if (*(msg->tail-1) == '\r' || *(msg->tail-1) == '\n')
>> +		*(msg->tail - 1) = '\0';
>> +	else {
>> +		LOGP(DMGCP, LOGL_ERROR, "Cannot NUL terminate MGCP message: "
>> +		     "Length: %d, Buffer size: %d\n",
>> +		     msgb_l2len(msg), msg->data_len);
>> +		return NULL;
>> +	}
> 
> The check misses if "tail - 1" is already \0
Yes, but does this hurt? I wouldn't expect l2 to end with a \0 since the
protocol is defined without one (see below). And even if there was one,
a second one wouldn't be a problem.

> and if tail - 1 is not NULL.
You mean that tail == (uint8_t *)1 ??? This mustn't happen anyway.

>
 I would just add an OSMO_ASSERT and fix the caller that didn't
> null terminate?! What do you think?

I don't agree that the caller has to NUL-terminate. It is an
implementation detail of mgcp_handle_message() that it uses string
parsing functions that depend on a terminating \0 character. The
caller provides the length of the data area and that just fine. Even
more it reserves a byte at the end of the buffer to have space for a \0,
which is more than the protocol requires. And it guarantees this way,
that the first condition (tailroom > 0) always succeeds, which is much
better IMO than inserting protocol violating garbage characters (the \0)
at the end of the buffer within the l2 data area.

What's about changing msgb to always write a \0 after the data area
every time tail gets modified? Like with QByteArray you could always
safely use string functions without handling this explicitely. But I'm
afraid there is some code already that relies of changing tail freely
without expecting this to cause this kind of side effect even when used
with put. Perhaps one can make this optional?

Jacob





More information about the OpenBSC mailing list