Juliusz Chroboczek <jch@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