[Xorp-hackers] Crash due to stale cached xrl sender pointer.

[Ben Greear]

On 10/29/2009 11:34 AM, Bruce Simpson wrote:
> Ben Greear wrote:
>> ...
>> Please note that the sender will be marked in-active, so the XRL will
>> not actually
>> try to use it, but if the memory is gone, then it can't even check the
>> foo->active()
>> flag w/out crashing.
>>
>> It seems a pretty simple use-after-free bug, and the fix seems pretty
>> trivial to me.
>
> I'm pleased that you've found an issue, and come up with a fix that
> appears to work for you in the here and now. I would also class part of
> the issue you've run into as a design bug in XRL, and have tried to
> explain (as best I can) why I believe that is the case.

If anything can ever delete a sender, and if we don't clean up outstanding
XRLs when we delete the sender, then the bug exists.  Grep for 'destroy_sender'
to see how xrl_router.cc can destroy them..because the are no longer 'live'.
Base-class doesn't handle setting 'aliveness', so no idea what object is actually
no longer thinking it is alive.

We either need to clean up those XRLs by invalidating their cache,
remove the xrl sender cache entirely, or make sure the sender can't
be deleted while XRLs referencing it exist.

The first is liable to be difficult.

The second a performance penalty.

The third used ref-ptrs and changed very little actual logic.

> I would prefer to know what the root cause of the transport pointer
> being invalidated is; this is mostly so that I can avoid introducing a
> similar situation in new code.
>
> However, I'm concerned that the suggested fix, actually makes the code
> more difficult to read than it already is. I'm not happy with ref_ptr,
> and it has been a source of problems for me in the past.

Xorp is a royal pain in the arse to read, with all it's typedefs, deep class inheritance,
auto-generated templated code (try to read the callback code some day..impossible),
timers, xrl black hole, chained (and unchained, for that matter) callbacks, etc.

It is one of the most hard to read pieces of code I've ever looked at (only the original
Vocal project was just barely worse, primarily because it was threaded and full of bugs).
Maybe Thrift will help..but if it's just yet another indirection or black hole of
magic, I doubt it.

> Of course, it's worth bearing in mind that I am looking at this from a
> very critical viewpoint at the moment. ;-)

I think if I spent more than 2 days looking at XRL I'd rip it's guts out and
re-implement it entirely.  I don't envy your task, and I hope it works out well.

Thanks,
Ben

-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com