[Babel-users] Babel: Possible segfault in bird unfeasible update handling code

[Toke Høiland-Jørgensen]

Juliusz Chroboczek <jch at irif.fr> writes:

>> The problematic bit is, I think, 's' in babel_handle_update can be NULL
>> because nothing ensures the babel_source for a particular neighbour
>> actually exists here:
>
> s will be passed to babel_is_feasible, which returns true if s is null.
> Later on, s is only used if feasible is false, in which case it cannot be
> null.
>
> See RFC 8966 Section 3.5.1:
>
>     a route advertisement carrying the quintuple (prefix, plen, router-id,
>     seqno, metric) is feasible if one of the following conditions holds:
>
>     • ...
>     • no entry exists in the source table indexed by (prefix, plen, router-id);
>     • ...
>
> I agree that the code is a little too subtle for comfort.

Pish posh, there's a totally-obvious comment saying /* for feasibility */ 
next to where 's' is assigned :P

>> Perhaps find should just be replaced by babel_get_source here?
>
> babel_get_source sets the seqno to an arbitrary value (0), so it should
> only be used if it is immediately followed by an assignment to s->seqno.
> A better API would be to pass the seqno to babel_get_source.

Hmm, yeah, possibly from a readability PoV, but, well, there's only the
single caller, so it becomes a bit redundant since we have to do the
check anyway afterwards.

And I don't think switching babel_handle_update() to use
babel_get_source() is a good idea either; we'd end up creating new
source objects and leave them to be garbage collected just to improve
readability a bit; just add a comment explaining why the deref is safe? :)

> (I haven't looked at it in detail, but the code in babel_send_update_ looks
> incorrect to me, by the way: it's comparing seqnos as integers, where it
> should be doing comparisons modulo 2¹⁶, as defined in Section 3.2.1.
> Toke?)

You're right about this, though; same in babel_is_feasible(). Nice
catch! Will send a patch...

-Toke