Avoiding msgb leaks is easiest if the caller retains ownership of the msgb. Take this hypothetical chain where leaks are obviously avoided:
void send() { msg = msgb_alloc(); dispatch(msg); msgb_free(msg); }
void dispatch(msg) { osmo_fsm_inst_dispatch(fi, msg); }
void fi_on_event(fi, data) { if (socket_is_ok) socket_write((struct msgb*)data); }
void socket_write(msgb) { if (!ok1) return; if (ok2) { if (!ok3) return; write(sock, msg->data); } }
However, if the caller passes ownership down to the msgb consumer, things become nightmarishly complex:
void send() { msg = msgb_alloc(); rc = dispatch(msg); /* dispatching event failed? */ if (rc) msgb_free(msg); }
int dispatch(msg) { if (osmo_fsm_inst_dispatch(fi, msg)) return -1; if (something_else()) return -1; // <-- double free! }
void fi_on_event(fi, data) { if (socket_is_ok) { socket_write((struct msgb*)data); else /* socket didn't consume? */ msgb_free(data); }
int socket_write(msgb) { if (!ok1) return -1; // <-- leak! if (ok2) { if (!ok3) goto out; write(sock, msg->data); } out: msgb_free(msg); return -2; }
If any link in this call chain fails to be aware of the importance to return a failed RC or to free a msgb if the chain is broken, we have a hidden msgb leak.
This is the case with osmo_sccp_user_sap_down(). In new osmo-msc, passing data through various FSM instances, there is high potential for leak/double-free bugs. A very large brain is required to track down every msgb path.
Isn't it possible to provide osmo_sccp_user_sap_down() in the caller-owns paradigm? Thinking about an osmo_sccp_user_sap_down2() that simply doesn't msgb_free().
Passing ownership to the consumer is imperative if a msg queue is involved that might send out asynchronously. (A workaround could be to copy the message before passing into the wqueue.) However, looks to me like no osmo_wqueue is involved in osmo_sccp_user_sap_down()? It already frees the msgb right upon returning, so this should be perfectly fine -- right?
I think I'll just try it, but if anyone knows a definite reason why this will not work, please let me know.
(Remotely related, I also still have this potential xua msg leak fix lying around, never got around to verify it: https://gerrit.osmocom.org/#/c/libosmo-sccp/+/9957/ )
~N
Hi Neels,
On Sun, Mar 10, 2019 at 04:31:05AM +0100, Neels Hofmeyr wrote:
Avoiding msgb leaks is easiest if the caller retains ownership of the msgb.
ACK.
However, if the caller passes ownership down to the msgb consumer, things become nightmarishly complex:
ACK.
If any link in this call chain fails to be aware of the importance to return a failed RC or to free a msgb if the chain is broken, we have a hidden msgb leak.
Indeed.
This is why I have been arguing in favor of something like a "packet dispatch talloc context" for some time. Unfortuantely I didn't have any time to work on this so far.
The proposal is along those lines: * we create a talloc context every time we drop out of the select() statement * any msgb's allocated from routines that read from sockets / etc. are allocated from that context * just before we return into osmo_select_main() [or in the start of that function], we free the entire talloc context, which will free any msgb's in it * if anyone wants to take ownership of a msgb for a longer time (e.g. enqueue it somewhere for delayed processing, ...) they need to re-parent the object away from the volatile "packet dispathc talloc context" and move it into another context. Talloc provides talloc_steal() or better talloc_move() for it.
I would think this is an elegant solution. That particular volatile talloc context would not be restricted to msgb's but could be used for any kind of 'temporary' allocations. We could lazily allocate whatever we wanted (like temporary string buffers, etc.) and know that it will be safely released once the current select dispatch ceases to exist. Sure, whenever we can we should still use the stack, as stack allocations are cheaper than heap allocations.
This is the case with osmo_sccp_user_sap_down(). In new osmo-msc, passing data through various FSM instances, there is high potential for leak/double-free bugs. A very large brain is required to track down every msgb path.
Let me know what you think of the above-mentioned proposal.
We could have something like msgb_alloc2() which takes an additional talloc context as argument. Pluse creating that [gloal variable] talloc context in the beginning of osmo_fd_disp_fds() and talloc_free()ing it at the end of osmo_fd_disp_fds() should do the trick.
One might evene be a able to do some trickery with talloc_set_destructor() to print a BIG FAT WARNING/ERROR message in case anyone ever tries to free memory allocated from this context manually. That destructor would simpl have to be removed or disabled when the select loop handling takes care of it.
Isn't it possible to provide osmo_sccp_user_sap_down() in the caller-owns paradigm? Thinking about an osmo_sccp_user_sap_down2() that simply doesn't msgb_free().
It's possible. But I think there are use cases for both behaviors.
Passing ownership to the consumer is imperative if a msg queue is involved that might send out asynchronously. (A workaround could be to copy the message before passing into the wqueue.)
The proposed approach would allow for "re-parenting" without having to copy the msgb or any of the metadata. Even the address of the msgb stays the same if you move/steal it.
I think I'll just try it, but if anyone knows a definite reason why this will not work, please let me know.
Not off my head, but I guess nobody is able to make guarantees on this without a thorough code audit and/or extensive testing. THat's why I like the idea of this volatile context.
However, I realize only now that you're referring not to the case of received message buffers, but about locally-originated msgbs. But for them, too, the rule could be simply something like this: * allocate memory from the volatile context * pass it into any library function * if the library function transmits it right away, no need to free/steal * if the library function enqueues it somewhere and doesn't immediately send it over a socket or the like, it needs to move/steal it.
Thinking of the above, I think the 'being queued' case is more or less the normal one, as we can never make the assumption that the underlying transport layer is able to accept transmission of any data in a non-blocking way. So there will always be some kind of write queue wherever we transmit.
The only situation where we can "normally" assume that the msgb of any outbound message can be free'd immediately is: * datagram sockets like for RTP * local primitives that are simply sent to another layer in the local stack, and which are released immediately after that stack has received them
Regards, Harald
On Tue, Mar 12, 2019 at 09:46:28PM +0100, Harald Welte wrote:
This is why I have been arguing in favor of something like a "packet dispatch talloc context" for some time. Unfortuantely I didn't have any time to work on this so far.
The proposal is along those lines:
- we create a talloc context every time we drop out of the select() statement
- any msgb's allocated from routines that read from sockets / etc. are allocated from that context
- just before we return into osmo_select_main() [or in the start of that function], we free the entire talloc context, which will free any msgb's in it
- if anyone wants to take ownership of a msgb for a longer time (e.g. enqueue it somewhere for delayed processing, ...) they need to re-parent the object away from the volatile "packet dispathc talloc context" and move it into another context. Talloc provides talloc_steal() or better talloc_move() for it.
I would think this is an elegant solution.
I fully agree, it's a great solution for two problems we have: msgb ownership as well as re-using foo_name() functions in the same print format.
One limitation to the talloc_steal(): assume some code path already stole from the volatile context, then it would be a bit rude to have another code path steal it from the first code path again. So maybe it would be good to be able to determine whether a msgb is still volatile or already owned, so that implementations can decide to only steal from the volatile context.
We could lazily allocate whatever we wanted (like temporary string buffers, etc.) and know that it will be safely released once the current select dispatch ceases to exist.
That is very desirable IMO. I've looked at some of your WIP patches in gerrit, and there placed a comment towards this. I would like to change the current string buffer returning functions under the hood without having to change the function signature -- after all, the callers don't need to care whether a buffer is static or from a volatile context. No caller anywhere is allowed to assume the returned string remains valid for a long time, so they do already strcpy() if they have to keep the returned string safe. The current patches look like we need to produce a lot of patches to use the volatile API... see https://gerrit.osmocom.org/#/c/libosmocore/+/13311/
use the stack whenever we can
One advantage of changing all string buffer functions to a volatile buffer would be that any static-buf-reuse bugs we might already have in our LOGP or printf() will be fixed implicitly, and will also be avoided for the future. I would very much like to not have to think about writing log statements so much, but make foo_name() invocations safe, period. After all, we don't log that much if LOGL_DEBUG is disabled, and for DEBUG switched on we are typically in a non-production environment.
If dynamic allocations are a problem, I could even imagine some array of 10 or so string buffers that are always kept around to avoid re-allocating too much for logging strings.
You catch my drift, I'm for changing the current implementation without needing explicit changes to calling code. I'm certain that you have good reasons to not want to do that, but are they worth the human time to use/maintain new API?
This is the case with osmo_sccp_user_sap_down().
Let me know what you think of the above-mentioned proposal.
It would make my other patch obsolete, in a good way. https://gerrit.osmocom.org/13277 osmo_sccp_user_sap_down2()
One might evene be a able to do some trickery with talloc_set_destructor() to print a BIG FAT WARNING/ERROR message in case anyone ever tries to free memory allocated from this context manually.
It's not harmful to free volatile bits ahead of time though. It is even desirable. If I want to use the current osmo_sccp_user_sap_down() as-is, I could just allocate a volatile msgb and feed that down the code path that ends up in osmo_sccp_user_sap_down(). If the msgb reaches all the way there, it gets freed within osmo_sccp_user_sap_down(), otherwise the volatile context will clean up the leftovers from error handling.
If we instead enforce that volatile contexts are not freed manually, then we would again need an osmo_sccp_user_sap_down2() API change; I assume that's not the only one.
However, I realize only now that you're referring not to the case of received message buffers, but about locally-originated msgbs. But for them, too, the rule could be simply something like this:
- allocate memory from the volatile context
- pass it into any library function
- if the library function transmits it right away, no need to free/steal
- if the library function enqueues it somewhere and doesn't immediately send it over a socket or the like, it needs to move/steal it.
exactly.
For changing existing enqueueing functions without needing a new API version, above idea would come in handy: determine whether the msgb is in the / a volatile context or not. If it is, steal, otherwise act as before.
Thinking of the above, I think the 'being queued' case is more or less the normal one, as we can never make the assumption that the underlying transport layer is able to accept transmission of any data in a non-blocking way. So there will always be some kind of write queue wherever we transmit.
In some cases we actually memcpy(), as I think osmo_sccp_user_sap_down() does. Otherwise it wouldn't be able to msgb_free() before returning, which it does, right?
- local primitives that are simply sent to another layer in the local stack, and which are released immediately after that stack has received them
^ yes, that's what osmo_sccp_user_sap_down() does, of course
~N
-
Harald Welte -
Neels Hofmeyr