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