Re: [Babel-users] Babel: Possible segfault in bird unfeasible update handling code
On Mon, Jan 30, 2023 at 11:10:28PM +0100, Toke Høiland-Jørgensen via Bird-users wrote: > Juliusz Chroboczek 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/96d7c4679df49b34be004177b10a99210af5f141 -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santi...@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." ___ Babel-users mailing list Babel-users@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/babel-users
Re: [Babel-users] Babel: Possible segfault in bird unfeasible update handling code
Juliusz Chroboczek 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 ___ Babel-users mailing list Babel-users@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/babel-users
Re: [Babel-users] Babel: Possible segfault in bird unfeasible update handling code
> 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 ___ Babel-users mailing list Babel-users@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/babel-users
[Babel-users] 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 ___ Babel-users mailing list Babel-users@alioth-lists.debian.net https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/babel-users
