I'm finding heap use after free related to osmo_fsm:
- When dispatching a timer_cb event, we want to set fi->T back to 0. (see
fsm_tmr_cb())
- So that we know which timer fired, we leave fi->T at its current value for
the timer_cb to be able to make decisions based on that. This is used for
example in fsm_timeout_cb() in osmo-bsc/osmo_bsc_mgcp.c.
- In other code, the timer_cb event may decide to fire events or call functions
which effect an FSM instance teardown and deallocation, meaning that in
fsm_tmr_cb(), we are *not* allowed to generally assume the fi still exists.
If we set fi->T = 0 after the timer_cb() was called, we may write to freed
mem.
- Also, if the timer_cb effects events with their own timeout, we will also
overwrite the fi->T value with zero after another timer has set its value in
T, causing failures to evaluate that timer later on.
Solutions so far:
- set fi->T = 0 first, and pass T to the timer_cb as arg instead. I think this
is the best and cleanest, but also the hardest in terms of backwards compat.
I have a patch that adds timer_cb2() that features a T arg; we need to submit
that, move all code that wants to use T to that timer_cb2(), and then we can
move the fi->T = 0 to before the timer_cb(). Actually, this is not being
backwards compatible at all, because all code trying to use fi->T as usual
will need adjustment, we break API either way and might as well add T to the
timer_cb directly instead... :/
- Actually not set fi->T = 0 at all. We will have T holding the value of the
timer that fired last, all old code will still find this T in fi->T, and
after the cb, we just leave it as it was. Does it really need to be cleared?
This has no compatibility issues and solves the use after free as well as
setting a new T from timer_cb() issues. Going for this one so far.
Thoughts?
~N
Show replies by date