Some additions below. On Wed, Mar 20, 2019 at 10:52:46AM +0100, Harald Welte wrote: I forgot here that this obviously only works if the msgb is created + consumed within the same select dispatch. If e.g. the msgb is enqueued on a transmit queue, you would have to talloc_steal() / talloc_reparent() it to a different context before returning from the select dispatch. In terms of our usual use cases, I see the following scenarios: a) incoming, locally handled message The msgb can normally be allocated from OTC_SELECT as we are processing the entire msgb within the select dispatch. If we end up queueing it somewhere, we need to steal it. b) incoming, forwarded message if we already know we'll forward it (e.g. in gb-proxy or bsc_nat), we might directly allocate it from OTC_GLOBAL or any of its children. c) outgoing message As we're operating in non-blocking mode, we can never make the assumption that any of the outbound sockets will be write-able. Hence, we always have a transmit queue, and as a result we have to allocate from OTC_GLOBAL or one of its children. d) local primitives (osmo_prim) As osmo_prim are msgb-wrapped, and primitives [at least for now] are always only between our local SAP user and provider (or vice versa) we might use OTC_SELECT here, and make sure that all users that want to recycle a osmo_prim msgb will steal it, if needed (I don't think that's a very valid use case). This applies both ways: for primitives from the provider to the user, as well as the other way around. Regards, Harald -- - Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)

Hi Pau, On Wed, Mar 20, 2019 at 12:54:11PM +0100, Pau Espin Pedrol wrote: What is your solution to neels' recently described problem with msgb ownership in successful/error cases when passing msgb's between different subsystems (in his example the libosmo-sccp sigtran stack) ? I think this is where the beauty of this system really shows. But maybe it's possible even without the 'select scoped context' with keeping allocations all global (like now) and using talloc_reference()? This way the default behavior would be an explicit free() by the caller, i.e. msg = msgb_alloc() /* put some data in the msgb, e.g. received from socket */ call_some_other_subsystem(msg); talloc_unlink(msg); /* cannot use {msgb,talloc}_free() anymore! */ And if that other_subsystem would want to keep msgb around, they would have to talloc_reference() the msgb, thereby having two parents. The msgb would only be free'd at the last talloc_unlink(). But that turns the talloc tree into a graph and to me it really messes up things. Also the fact that you can no longer use talloc_free() makes things very complicated. So not really an option. -- - Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)

Talking of features, another idea comes to mind: freeing of FSM instances. I more often than not have the problem that a child FSM is freeing, telling the parent about it, then the parent decides to free because the child's result signalled some end, and then we have a double free because the child was allocated with the parent as talloc context, the parent has just deallocated, but after the child's cleanup is through, the fsm code wants to also deallocate the child. In these situations I so far need to introduce checks that prevent the parent from freeing until the children are completely finished with their actions -- for example, avoid signalling end from the cleanup function or event handling yet, instead completely rely on the parent_term_event, which is dispatched only after the child was freed. With a volatile context, when an FSM instance should go away I could reparent it to the volatile context, it will be cleaned up once all event handling is through, and I avoid running into double free by accident. It would probably have to be integrated in the fsm.c deallocation code. On Wed, Mar 20, 2019 at 12:54:11PM +0100, Pau Espin Pedrol wrote: On the contrary, it might require some thinking about contexts once at a few pivotal points -- AFAICT it only becomes complex with wqueues, and I have the impression we could fairly easily handle this only once in the wqueue API (reparent when adding to or removing from a wqueue) so that callers are actually going to be relieved of all thinking about msgb scoping. The benefit on the other hand: I am quite sure that we still have a whole score of mem leaks / use after free bugs in lots of error handling code paths, which no-one has noticed and no-one is fixing. With a volatile talloc context approach we can swat *all* of those bugs by design. We trade a limited amount of brain work for, theoretically, an inifnite amount of hidden bugs getting fixed. These static buffers have so many various ways of shooting yourself in the foot, it is very desirable to get rid of them. The idea for me is to achieve less complexity in using the API. A volatile context is the means to that. ...are we talking about a libosmocore 2.0 now? ~N

On Thu, Mar 21, 2019 at 09:30:28AM +0100, Harald Welte wrote: Just after writing the above, I have introduced another deallocation failure mode in the cleanup among RTP-stream/call leg/MGW endpoint/MSC subscriber FSMs, and am fed up with solving these ad hoc as they show up. So now I am writing an explicit FSM deallocation test, to figure out how to properly solve various cleanup scenarios -- so that I can hopefully apply one and the same mechanism to all FSM implementations. (This might get a bit complex to follow, a proposed solution is in the bottom.) Here is the test: branch neels/fsm_dealloc_test of libosmocore http://git.osmocom.org/libosmocore/commit/?h=neels/fsm_dealloc_test&id=… The "scene" struct shows a number of objects, but have reduced to only three being in use. branch0 <-parent/child-> twig0a \ / ----- other ------- So both a parent and child FSM reference some "other" object which they depend upon. (A real world equivalent is that both a call_leg and rtp_stream FSM depend on the MGW endpoint -- in which situation also all of those are children of an MSC subscriber FSM, just saying complexity is easily added in real life) The underlying problem here is: - when the child has an error and goes away, the parent should go away as well. So there is an event from the child to the parent saying "I am gone, you too go now": EV_CHILD_GONE. - if now I tell branch0 to deallocate regularly, it first tells the child to deallocate. The child then also says EV_CHILD_GONE. - The parent receives EV_CHILD_GONE and again triggers its own deallocation. Now there are two osmo_fsm_inst_term() called on the same FSM instance. A way to fix this is to introduce a ST_DESTROYING state. It acts as a flag to prevent re-triggering an osmo_fsm_inst_term(). So if as soon as once in ST_DESTROYING, it will no longer issue osmo_fsm_inst_term() on itself. My test models this structure. In the test code try in turns to deallocate with a EV_DESTROY event, and then a direct osmo_fsm_inst_term(). Both of them should ideally be safe to do, because on weird errors anything should be able to just term an FSM instance. Dispatching an EV_DESTROY actually works out fine. But calling osmo_fsm_inst_term() directly fails. The difference is that in the EV_DESTROY case, the branch0 FSM has changed to state ST_DESTROYING, and if it receives events that peer objects are gone, it ignores them. In the term case, the FSM instance stays in the "ALIVE" state, in which case a term triggered at the child twig0a in turn dispatches an EV_CHILD_GONE, which the parent branch0 acts upon by transitioning to the ST_DESTROYING state, which launches a *second* osmo_fsm_inst_term() on itself. The output looks like this -- first the successful run, then the failing one: ▶ ./fsm_dealloc_test DLGLOBAL DEBUG scene_alloc() DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0) DLGLOBAL DEBUG test(other){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0 DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a DLGLOBAL DEBUG scene_alloc() DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0) DLGLOBAL DEBUG test(other){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0 DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a DLGLOBAL DEBUG scene_alloc() DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0) DLGLOBAL DEBUG test(other){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0 DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a DLGLOBAL DEBUG ---------------------- test_destroy(branch0) DLGLOBAL DEBUG --- before destroy cascade, got: branch0 twig0a other DLGLOBAL DEBUG --- DLGLOBAL DEBUG test(branch0){alive}: Received Event EV_DESTROY DLGLOBAL DEBUG 1 (branch0.alive()) DLGLOBAL DEBUG test(branch0){alive}: alive(EV_DESTROY) DLGLOBAL DEBUG test(branch0){alive}: state_chg to destroying DLGLOBAL DEBUG 2 (branch0.alive(),branch0.destroying_onenter()) DLGLOBAL DEBUG test(branch0){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(branch0){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) DLGLOBAL DEBUG test(branch0){destroying}: pre_term() DLGLOBAL DEBUG test(twig0a){alive}: Terminating (cause = OSMO_FSM_TERM_PARENT) DLGLOBAL DEBUG test(twig0a){alive}: pre_term() DLGLOBAL DEBUG test(twig0a){alive}: Removing from parent test(branch0) DLGLOBAL DEBUG 3 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup()) DLGLOBAL DEBUG test(twig0a){alive}: cleanup() DLGLOBAL DEBUG test(twig0a){alive}: scene forgets twig0a DLGLOBAL DEBUG test(twig0a){alive}: removing reference twig0a.other[0] -> other DLGLOBAL DEBUG test(other){alive}: Received Event EV_OTHER_GONE DLGLOBAL DEBUG 4 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG test(other){alive}: alive(EV_OTHER_GONE) DLGLOBAL DEBUG 5 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.other_gone()) DLGLOBAL DEBUG test(other){alive}: Dropped reference other.other[1] = twig0a DLGLOBAL DEBUG 4 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG test(other){alive}: state_chg to destroying DLGLOBAL DEBUG 5 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter()) DLGLOBAL DEBUG test(other){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(other){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) DLGLOBAL DEBUG test(other){destroying}: pre_term() DLGLOBAL DEBUG 6 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup()) DLGLOBAL DEBUG test(other){destroying}: cleanup() DLGLOBAL DEBUG test(other){destroying}: scene forgets other DLGLOBAL DEBUG test(other){destroying}: removing reference other.other[0] -> branch0 DLGLOBAL DEBUG test(branch0){destroying}: Received Event EV_OTHER_GONE DLGLOBAL DEBUG 7 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branc DLGLOBAL DEBUG test(branch0){destroying}: destroying(EV_OTHER_GONE) DLGLOBAL DEBUG 8 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branc DLGLOBAL DEBUG test(branch0){destroying}: Dropped reference branch0.other[0] = other DLGLOBAL DEBUG 7 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branc DLGLOBAL DEBUG 6 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup()) DLGLOBAL DEBUG test(other){destroying}: cleanup() done DLGLOBAL DEBUG 5 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter()) DLGLOBAL DEBUG test(other){destroying}: Freeing instance DLGLOBAL DEBUG test(other){destroying}: Deallocated DLGLOBAL DEBUG 4 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG 3 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup()) DLGLOBAL DEBUG test(twig0a){alive}: cleanup() done DLGLOBAL DEBUG 2 (branch0.alive(),branch0.destroying_onenter()) DLGLOBAL DEBUG test(twig0a){alive}: Freeing instance DLGLOBAL DEBUG test(twig0a){alive}: Deallocated DLGLOBAL DEBUG 3 (branch0.alive(),branch0.destroying_onenter(),branch0.cleanup()) DLGLOBAL DEBUG test(branch0){destroying}: cleanup() DLGLOBAL DEBUG test(branch0){destroying}: scene forgets branch0 DLGLOBAL DEBUG test(branch0){destroying}: cleanup() done DLGLOBAL DEBUG 2 (branch0.alive(),branch0.destroying_onenter()) DLGLOBAL DEBUG test(branch0){destroying}: Freeing instance DLGLOBAL DEBUG test(branch0){destroying}: Deallocated DLGLOBAL DEBUG 1 (branch0.alive()) DLGLOBAL DEBUG 0 (-) DLGLOBAL DEBUG --- after destroy cascade, still got: <------ SUCCESS! DLGLOBAL DEBUG --- cleaning up DLGLOBAL DEBUG scene_alloc() DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0) DLGLOBAL DEBUG test(other){alive}: Allocated DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0 DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a DLGLOBAL DEBUG ---------------------- test_term(branch0) DLGLOBAL DEBUG --- before term cascade, got: branch0 twig0a other DLGLOBAL DEBUG --- DLGLOBAL DEBUG test(branch0){alive}: Terminating (cause = OSMO_FSM_TERM_REGULAR) <------------ branch0 terminating DLGLOBAL DEBUG test(branch0){alive}: pre_term() DLGLOBAL DEBUG test(twig0a){alive}: Terminating (cause = OSMO_FSM_TERM_PARENT) DLGLOBAL DEBUG test(twig0a){alive}: pre_term() DLGLOBAL DEBUG test(twig0a){alive}: Removing from parent test(branch0) DLGLOBAL DEBUG 1 (twig0a.cleanup()) DLGLOBAL DEBUG test(twig0a){alive}: cleanup() DLGLOBAL DEBUG test(twig0a){alive}: scene forgets twig0a DLGLOBAL DEBUG test(twig0a){alive}: removing reference twig0a.other[0] -> other DLGLOBAL DEBUG test(other){alive}: Received Event EV_OTHER_GONE DLGLOBAL DEBUG 2 (twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG test(other){alive}: alive(EV_OTHER_GONE) DLGLOBAL DEBUG 3 (twig0a.cleanup(),other.alive(),other.other_gone()) DLGLOBAL DEBUG test(other){alive}: Dropped reference other.other[1] = twig0a DLGLOBAL DEBUG 2 (twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG test(other){alive}: state_chg to destroying DLGLOBAL DEBUG 3 (twig0a.cleanup(),other.alive(),other.destroying_onenter()) DLGLOBAL DEBUG test(other){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(other){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) DLGLOBAL DEBUG test(other){destroying}: pre_term() DLGLOBAL DEBUG 4 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup()) DLGLOBAL DEBUG test(other){destroying}: cleanup() DLGLOBAL DEBUG test(other){destroying}: scene forgets other DLGLOBAL DEBUG test(other){destroying}: removing reference other.other[0] -> branch0 DLGLOBAL DEBUG test(branch0){alive}: Received Event EV_OTHER_GONE DLGLOBAL DEBUG 5 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive()) DLGLOBAL DEBUG test(branch0){alive}: alive(EV_OTHER_GONE) DLGLOBAL DEBUG 6 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.other_gone()) DLGLOBAL DEBUG test(branch0){alive}: Dropped reference branch0.other[0] = other DLGLOBAL DEBUG 5 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive()) DLGLOBAL DEBUG test(branch0){alive}: state_chg to destroying DLGLOBAL DEBUG 6 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.destroying_onenter()) DLGLOBAL DEBUG test(branch0){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(branch0){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) <-------- branch0 terminating AGAIN!?!? DLGLOBAL DEBUG test(branch0){destroying}: pre_term() DLGLOBAL DEBUG 7 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.destroying_onenter(),branc DLGLOBAL DEBUG test(branch0){destroying}: cleanup() DLGLOBAL DEBUG test(branch0){destroying}: scene forgets branch0 DLGLOBAL DEBUG test(branch0){destroying}: cleanup() done DLGLOBAL DEBUG 6 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.destroying_onenter()) DLGLOBAL DEBUG test(branch0){destroying}: Freeing instance DLGLOBAL DEBUG test(branch0){destroying}: Deallocated DLGLOBAL DEBUG 5 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive()) DLGLOBAL DEBUG 4 (twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup()) DLGLOBAL DEBUG test(other){destroying}: cleanup() done DLGLOBAL DEBUG 3 (twig0a.cleanup(),other.alive(),other.destroying_onenter()) DLGLOBAL DEBUG test(other){destroying}: Freeing instance DLGLOBAL DEBUG test(other){destroying}: Deallocated ================================================================= ==9774==ERROR: AddressSanitizer: heap-use-after-free on address 0x612000001128 at pc 0x7f74d8b8f249 bp 0x7ffebeffe490 sp 0x7ffebeffe488 WRITE of size 8 at 0x612000001128 thread T0 #0 0x7f74d8b8f248 in __llist_del ../../../src/libosmocore/include/osmocom/core/linuxlist.h:114 #1 0x7f74d8b8f380 in llist_del ../../../src/libosmocore/include/osmocom/core/linuxlist.h:126 #2 0x7f74d8b93eaa in osmo_fsm_inst_free ../../../src/libosmocore/src/fsm.c:404 #3 0x7f74d8b9ba9c in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:738 #4 0x563b17db94b5 in destroying_onenter ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:173 #5 0x7f74d8b987be in state_chg ../../../src/libosmocore/src/fsm.c:521 #6 0x7f74d8b98870 in _osmo_fsm_inst_state_chg ../../../src/libosmocore/src/fsm.c:577 #7 0x563b17db8db8 in alive ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:147 #8 0x7f74d8b99e2f in _osmo_fsm_inst_dispatch ../../../src/libosmocore/src/fsm.c:685 #9 0x563b17dbabc0 in cleanup ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:251 #10 0x7f74d8b9b292 in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:733 #11 0x7f74d8b9c1b1 in _osmo_fsm_inst_term_children ../../../src/libosmocore/src/fsm.c:784 #12 0x7f74d8b9a85e in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:720 #13 0x563b17dbd09d in obj_term ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:392 #14 0x563b17dbd8fb in test_term ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:425 #15 0x563b17dbdb62 in main ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:453 #16 0x7f74d7db909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) #17 0x563b17db5319 in _start (/n/s/dev/make/libosmocore/tests/fsm/fsm_dealloc_test+0x10319) Fixing this particular run can be done by manually setting this fi->state = ST_DESTROYING (without osmo_fsm_inst_state_chg(), since that would run the onenter() function and trigger an osmo_fsm_inst_term()). So I did: void pre_term(struct osmo_fsm_inst *fi, enum osmo_fsm_term_cause cause) { + /* sneak into "destroying" state without triggering events */ + fi->state = ST_DESTROYING; LOGPFSML(fi, LOGL_DEBUG, "%s()\n", __func__); } That's not so nice, changing states in a cheating way. A different solution could be some fsm.c internal "already terminating" flag, so I can call osmo_fsm_inst_term() any number of times, and only the first call will have an effect. One might think then the problem is solved? But then I still face this problem: - If the child is sent an EV_DESTROY, it goes into osmo_fsm_inst_term(). - As part of its cleanup, it terminates its reference to the "other" object. - Out of its own free will, the other object then decides that this means some problem appeared and decides to deallocate as well. - But the parent object also has a reference to "other". Since "other" is deallocating, it notifies the parent branch0 that it is now gone. - branch0 decides that if "other" has a problem, it is no longer useful and deallocates. - twig0a is *not* signalled to terminate from the parent branch0, because it has already removed itself from the parent at the start of its termination. - But branch0 calls talloc_free() on itself. Since twig0a was a talloc child of branch0, its memory storage is now gone. - All of the above happened while telling "other" that the twig0a is about to go. Now this code path wants to continue after the twig0a.cleanup(). Alas, its memory is no longer valid and the fsm.c termination code causes a use-after-free. Looks like this: DLGLOBAL DEBUG ---------------------- test_destroy(twig0a) DLGLOBAL DEBUG --- before destroy cascade, got: branch0 twig0a other DLGLOBAL DEBUG --- DLGLOBAL DEBUG test(twig0a){alive}: Received Event EV_DESTROY DLGLOBAL DEBUG 1 (twig0a.alive()) DLGLOBAL DEBUG test(twig0a){alive}: alive(EV_DESTROY) DLGLOBAL DEBUG test(twig0a){alive}: state_chg to destroying DLGLOBAL DEBUG 2 (twig0a.alive(),twig0a.destroying_onenter()) DLGLOBAL DEBUG test(twig0a){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(twig0a){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) DLGLOBAL DEBUG test(twig0a){destroying}: pre_term() DLGLOBAL DEBUG test(twig0a){destroying}: Removing from parent test(branch0) DLGLOBAL DEBUG 3 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup()) DLGLOBAL DEBUG test(twig0a){destroying}: cleanup() DLGLOBAL DEBUG test(twig0a){destroying}: scene forgets twig0a DLGLOBAL DEBUG test(twig0a){destroying}: removing reference twig0a.other[0] -> other <----- twig0a tells "other" that it is going DLGLOBAL DEBUG test(other){alive}: Received Event EV_OTHER_GONE DLGLOBAL DEBUG 4 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG test(other){alive}: alive(EV_OTHER_GONE) DLGLOBAL DEBUG 5 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.other_gone()) DLGLOBAL DEBUG test(other){alive}: Dropped reference other.other[1] = twig0a DLGLOBAL DEBUG 4 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive()) DLGLOBAL DEBUG test(other){alive}: state_chg to destroying DLGLOBAL DEBUG 5 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter()) DLGLOBAL DEBUG test(other){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(other){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) DLGLOBAL DEBUG test(other){destroying}: pre_term() DLGLOBAL DEBUG 6 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup()) DLGLOBAL DEBUG test(other){destroying}: cleanup() DLGLOBAL DEBUG test(other){destroying}: scene forgets other DLGLOBAL DEBUG test(other){destroying}: removing reference other.other[0] -> branch0 DLGLOBAL DEBUG test(branch0){alive}: Received Event EV_OTHER_GONE <----- branch0 gets told "other" is gone DLGLOBAL DEBUG 7 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG test(branch0){alive}: alive(EV_OTHER_GONE) DLGLOBAL DEBUG 8 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG test(branch0){alive}: Dropped reference branch0.other[0] = other DLGLOBAL DEBUG 7 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG test(branch0){alive}: state_chg to destroying DLGLOBAL DEBUG 8 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG test(branch0){destroying}: destroying_onenter() from alive DLGLOBAL DEBUG test(branch0){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR) DLGLOBAL DEBUG test(branch0){destroying}: pre_term() DLGLOBAL DEBUG 9 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG test(branch0){destroying}: cleanup() DLGLOBAL DEBUG test(branch0){destroying}: scene forgets branch0 DLGLOBAL DEBUG test(branch0){destroying}: cleanup() done DLGLOBAL DEBUG 8 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG test(branch0){destroying}: Freeing instance DLGLOBAL DEBUG test(branch0){destroying}: Deallocated <----- branch0 calls talloc_free() DLGLOBAL DEBUG 7 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0 DLGLOBAL DEBUG 6 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup()) DLGLOBAL DEBUG test(other){destroying}: cleanup() done DLGLOBAL DEBUG 5 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter()) DLGLOBAL DEBUG test(other){destroying}: Freeing instance DLGLOBAL DEBUG test(other){destroying}: Deallocated <---- twig0a.cleanup() is done, fsm.c wants to continue termination... ================================================================= ==16426==ERROR: AddressSanitizer: heap-use-after-free on address 0x6120000015a8 at pc 0x7faea5cb4149 bp 0x7fff936ffed0 sp 0x7fff936ffec8 WRITE of size 8 at 0x6120000015a8 thread T0 #0 0x7faea5cb4148 in __llist_del ../../../src/libosmocore/include/osmocom/core/linuxlist.h:114 #1 0x7faea5cb4280 in llist_del ../../../src/libosmocore/include/osmocom/core/linuxlist.h:126 #2 0x7faea5cb8daa in osmo_fsm_inst_free ../../../src/libosmocore/src/fsm.c:404 #3 0x7faea5cc099c in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:738 #4 0x56367132a4c5 in destroying_onenter ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:173 #5 0x7faea5cbd6be in state_chg ../../../src/libosmocore/src/fsm.c:521 #6 0x7faea5cbd770 in _osmo_fsm_inst_state_chg ../../../src/libosmocore/src/fsm.c:577 #7 0x563671329dc8 in alive ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:147 #8 0x7faea5cbed2f in _osmo_fsm_inst_dispatch ../../../src/libosmocore/src/fsm.c:685 #9 0x56367132bbd0 in cleanup ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:251 #10 0x7faea5cc0192 in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:733 #11 0x56367132a4c5 in destroying_onenter ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:173 #12 0x7faea5cbd6be in state_chg ../../../src/libosmocore/src/fsm.c:521 #13 0x7faea5cbd770 in _osmo_fsm_inst_state_chg ../../../src/libosmocore/src/fsm.c:577 #14 0x563671329e3d in alive ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:159 #15 0x7faea5cbed2f in _osmo_fsm_inst_dispatch ../../../src/libosmocore/src/fsm.c:685 #16 0x56367132e0be in obj_destroy ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:389 #17 0x56367132e519 in test_destroy ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:408 #18 0x56367132ebf4 in main ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:454 #19 0x7faea4edf09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) #20 0x563671326329 in _start (/n/s/dev/make/libosmocore/tests/fsm/fsm_dealloc_test+0x10329) So, even though twig0a sends the EV_CHILD_GONE to its parent FSM only *after* all terminating is done, some events crossing over via "other" objects can still trigger the parent to also deallocate at the same time. Here the problem merely is that the child is allocated from the parent's talloc context. Had it been allocated from an independent talloc context, then there would not be any use-after-free. So let's try that: fi = osmo_fsm_inst_alloc_child(&test_fsm, parent->fi, EV_CHILD_GONE); OSMO_ASSERT(fi); + talloc_steal(s, fi); Now the next problem arises: when the child is done deallocating, fsm.c still has the parent FSM pointer to which it must still dispatch the parent_term event. When there is only a parent and a child involved, using the parent_term event actually solves this problem: if we were to dispatch EV_CHILD_GONE from the child's cleanup() function, then the parent would deallocate and free the child along with its own talloc context. By using the parent_term event, we wait until after cleanup() and is done and until after talloc_free() on the child is done, and only then dispatch EV_CHILD_GONE to the parent. But because a third "other" object is involved, the deallocation triger ricochets across that other object. It is out of scope to tell that other object to care about which two callers are using it and why they must not be told to deallocate in certain situations. It is certainly possible to do that, but it requires a lot of careful thinking through all possible variations, which is very easy to get wrong. The vision for fixing these kinds of situations is to avoid a use-after-free completely by keeping the talloc contexts around; Also all event dispatch and osmo_fsm_inst_term() calls must be stopped when a fi is marked to be released. - add flag osmo_fsm_inst.deallocating. If true, fsm.c denies all action triggers on the FSM instance (no osmo_fsm_inst_term(), no osmo_fsm_inst_state_chg(), no osmo_fsm_inst_dispatch()). - add a talloc bucket into which to reparent FSM instances instead of deallocating directly. Then each parent/child/"other" object that wants to trigger things on it will be able to see the "deallocating" flag, and still even use memory like the fi->id for logging. This talloc bucket could be a volatile select context, or it could even be a global context that exists once in fsm.c, the first osmo_fsm_inst_term() creates it, other callers add and remove use counts, and as soon as the last use count is removed, the entire context gets freed. I think this would solve all of the above problems. ~N

On Sun, Mar 24, 2019 at 06:54:15AM +0100, Neels Hofmeyr wrote: Interesting to note here is that I can apparently steal a talloc parent to become a talloc child (this is the result of the child osmo_fsm_inst_term() also causing the parent to term) parent_fi | +- child_fi talloc_steal(new_ctx=child_fi, parent_fi) I guess this should result in: child_fi | +- parent_fi I'm not entirely clear how this works out. Are those then still attached to whatever parent_fi had as a parent context? Are they floating alone? The test shows that it does work, no leaks, no loops. Since child_fi is guaranteed to be deallocated, either way would be fine. If this is bugging us we can use a different talloc_ctx to steal into -- I just wanted to avoid allocating another short-lived ctx. ~N