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

[Holger Hans Peter Freyther]

On Mon, Dec 02, 2013 at 10:21:58AM +0100, Jacob Erlbeck wrote:

> >> +	/* 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 {
> >> +		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.

tailroom == 0 AND  msg->l2h[msgb_length(msg) - 1] == '\0' is rejected
by your above code.


> You mean that tail == (uint8_t *)1 ??? This mustn't happen anyway.

true. I had tail not being initialized in mind. But that can not
happen.


> 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

Fair enough. I just noticed that the check above is a bit of code and
I think it missed one case, so a OSMO_ASSERT is easier to get right.



> 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?

In theory we always over allocate the msgb and due the usage of
talloc_zero should make sure that we have many NULLs at the end. If we
want to implement the extra reservation we have a bit of a problem. We
generally use a power of two for the allocation (in the hope we get a
full page). So if we do:

  +1: We certainly need to allocate more than a page
  -1: We might not have enough data.


For now. Please change the check to use l2h, msgb_length and handle
a NULL terminated input with no tailroom.


kind regards
	holger