On Wed, Apr 19, 2023 at 11:00:46AM +0200, Claudio Jeker wrote:
> I want to use this code also in bgpctl (like util.c) and since bgpctl
> has no fatalx() and "library" code should not abort.
>
> The comparison function can not return an error so instead sort invalid
> objects in a deterministic way. flowspec_cmp() should only be called on
> flowspec NLRI that have previously passed flowspec_valid(). Doing that is
> enough to ensure that neither extract_prefix() nor
> flowspec_next_component() fail.
OpenSSL solved this conundrum by returning -2 from some comparison
functions. I wonder why you did not choose do that here...
> The overflow check in flowspec_valid() is mostly an assert stile check but
> it is totally fine to return an error there.
Agreed.
ok tb
> --
> :wq Claudio
>
> Index: flowspec.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/flowspec.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 flowspec.c
> --- flowspec.c 19 Apr 2023 07:07:58 -0000 1.3
> +++ flowspec.c 19 Apr 2023 08:53:34 -0000
> @@ -105,10 +105,8 @@ flowspec_cmp_prefix4(const uint8_t *abuf
> clen = MINIMUM(alen, blen);
>
> /* only extract the common prefix */
> - if (extract_prefix(abuf + 2, ablen - 2, &a, clen, sizeof(a)) == -1)
> - fatalx("bad flowspec prefix encoding");
> - if (extract_prefix(bbuf + 2, bblen - 2, &b, clen, sizeof(b)) == -1)
> - fatalx("bad flowspec prefix encoding");
> + extract_prefix(abuf + 2, ablen - 2, &a, clen, sizeof(a));
> + extract_prefix(bbuf + 2, bblen - 2, &b, clen, sizeof(b));
>
> /* lowest IP value has precedence */
> cmp = memcmp(a, b, sizeof(a));
> @@ -149,10 +147,8 @@ flowspec_cmp_prefix6(const uint8_t *abuf
> clen = MINIMUM(alen, blen);
>
> /* only extract the common prefix */
> - if (extract_prefix(abuf + 3, ablen - 3, &a, clen, sizeof(a)) == -1)
> - fatalx("bad flowspec prefix encoding");
> - if (extract_prefix(bbuf + 3, bblen - 3, &b, clen, sizeof(b)) == -1)
> - fatalx("bad flowspec prefix encoding");
> + extract_prefix(abuf + 3, ablen - 3, &a, clen, sizeof(a));
> + extract_prefix(bbuf + 3, bblen - 3, &b, clen, sizeof(b));
>
> /* lowest IP value has precedence */
> cmp = memcmp(a, b, sizeof(a));
> @@ -193,7 +189,7 @@ flowspec_valid(const uint8_t *buf, int l
> len -= l;
> }
> if (len < 0)
> - fatalx("flowspec overflow");
> + return -1;
> return 0;
> }
>
> @@ -211,11 +207,12 @@ flowspec_cmp(const uint8_t *a, int alen,
>
> while (alen > 0 && blen > 0) {
> acomplen = flowspec_next_component(a, alen, is_v6, &atype);
> - if (acomplen == -1)
> - fatalx("bad flowspec component");
> bcomplen = flowspec_next_component(b, blen, is_v6, &btype);
> + /* should not happen */
> if (acomplen == -1)
> - fatalx("bad flowspec component");
> + return 1;
> + if (bcomplen == -1)
> + return -1;
>
> /* If types differ, lowest type wins. */
> if (atype < btype)
>
