Babel: Possible segfault in bird unfeasible update handling code
Hi Babelers, I've been working on the babel proto in bird and found some code where I can't convince myself it won't segfault. 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: /* Regular update */ [...] s = babel_find_source(e, msg->router_id); /* for feasibility */ [...] /* RFC section 3.8.2.2 - Dealing with unfeasible updates */ if (!feasible && (metric != BABEL_INFINITY) && (!best || (r == best) || (metric < best->metric))) babel_add_seqno_request(p, e, s->router_id, s->seqno + 1, 0, nbr); //^ BUG: Can 's' be NULL here? The only place that allocates sources is babel_send_update_ which just happens at it's own pace and has nothing to do with incoming update handling AFAICT. Am I missing something or is this a bug? Perhaps find should just be replaced by babel_get_source here? --Daniel
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.
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. (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?) -- Juliusz
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
On Mon, Jan 30, 2023 at 11:10:28PM +0100, Toke Høiland-Jørgensen via Bird-users wrote:
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.
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
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? :)
Added comment and unnecessary check (it will be likely eliminated anyways). https://gitlab.nic.cz/labs/bird/-/commit/96d7c4679df49b34be004177b10a99210af... -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
participants (4)
-
Daniel Gröber -
Juliusz Chroboczek -
Ondrej Zajicek -
Toke Høiland-Jørgensen