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