*: Fix usage of bfd_adj_event

Valgrind reports:

469901-==469901==
469901-==469901== Conditional jump or move depends on uninitialised value(s)
469901:==469901==    at 0x3A090D: bgp_bfd_dest_update (bgp_bfd.c:416)
469901-==469901==    by 0x497469E: zclient_read (zclient.c:3701)
469901-==469901==    by 0x4955AEC: thread_call (thread.c:1684)
469901-==469901==    by 0x48FF64E: frr_run (libfrr.c:1126)
469901-==469901==    by 0x213AB3: main (bgp_main.c:540)
469901-==469901==  Uninitialised value was created by a stack allocation
469901:==469901==    at 0x3A0725: bgp_bfd_dest_update (bgp_bfd.c:376)
469901-==469901==
469901-==469901== Conditional jump or move depends on uninitialised value(s)
469901:==469901==    at 0x3A093C: bgp_bfd_dest_update (bgp_bfd.c:421)
469901-==469901==    by 0x497469E: zclient_read (zclient.c:3701)
469901-==469901==    by 0x4955AEC: thread_call (thread.c:1684)
469901-==469901==    by 0x48FF64E: frr_run (libfrr.c:1126)
469901-==469901==    by 0x213AB3: main (bgp_main.c:540)
469901-==469901==  Uninitialised value was created by a stack allocation
469901:==469901==    at 0x3A0725: bgp_bfd_dest_update (bgp_bfd.c:376)

On looking at bgp_bfd_dest_update the function call into bfd_get_peer_info
when it fails to lookup the ifindex ifp pointer just returns leaving
the dest and src prefix pointers pointing to whatever was passed in.

Let's do two things:

a) The src pointer was sometimes assumed to be passed in and sometimes not.
Forget that.  Make it always be passed in
b) memset the src and dst pointers to be all zeros.  Then when we look
at either of the pointers we are not making decisions based upon random
data in the pointers.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>

diff --git a/isisd/isis_bfd.c b/isisd/isis_bfd.c
index f81dd6cf51c35f6eee8b21835dbdc82e6fb80a18..f6255d5a596756e1614b5f81ce887db7785946e2 100644 (file)
--- a/isisd/isis_bfd.c
+++ b/isisd/isis_bfd.c
@@ -146,11 +146,11 @@ static void bfd_adj_event(struct isis_adjacency *adj, struct prefix *dst,
 static int isis_bfd_interface_dest_update(ZAPI_CALLBACK_ARGS)
 {
        struct interface *ifp;
-       struct prefix dst_ip;
+       struct prefix dst_ip, src_ip;
        int status;
 
-       ifp = bfd_get_peer_info(zclient->ibuf, &dst_ip, NULL, &status,
-                               NULL, vrf_id);
+       ifp = bfd_get_peer_info(zclient->ibuf, &dst_ip, &src_ip, &status, NULL,
+                               vrf_id);
        if (!ifp || (dst_ip.family != AF_INET && dst_ip.family != AF_INET6))
                return 0;
 

diff --git a/lib/bfd.c b/lib/bfd.c
index 758271b729168f356a7417740ed9b78a0c430fca..a309b741ddbf2d8c0b3a3fda42821b9778d681e8 100644 (file)
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -224,6 +224,17 @@ struct interface *bfd_get_peer_info(struct stream *s, struct prefix *dp,
        int plen;
        int local_remote_cbit;
 
+       /*
+        * If the ifindex lookup fails the
+        * rest of the data in the stream is
+        * not read.  All examples of this function
+        * call immediately use the dp->family which
+        * is not good.  Ensure we are not using
+        * random data
+        */
+       memset(dp, 0, sizeof(*dp));
+       memset(sp, 0, sizeof(*sp));
+
        /* Get interface index. */
        ifindex = stream_getl(s);
 
@@ -249,13 +260,12 @@ struct interface *bfd_get_peer_info(struct stream *s, struct prefix *dp,
        /* Get BFD status. */
        *status = stream_getl(s);
 
-       if (sp) {
-               sp->family = stream_getc(s);
+       sp->family = stream_getc(s);
+
+       plen = prefix_blen(sp);
+       stream_get(&sp->u.prefix, s, plen);
+       sp->prefixlen = stream_getc(s);
 
-               plen = prefix_blen(sp);
-               stream_get(&sp->u.prefix, s, plen);
-               sp->prefixlen = stream_getc(s);
-       }
        local_remote_cbit = stream_getc(s);
        if (remote_cbit)
                *remote_cbit = local_remote_cbit;

diff --git a/ospfd/ospf_bfd.c b/ospfd/ospf_bfd.c
index d2c5090f2fc293ebc6382a0e3a46bc92acc48cba..cdc1d14febd1666593fd0d5fce1248dbb9039732 100644 (file)
--- a/ospfd/ospf_bfd.c
+++ b/ospfd/ospf_bfd.c
@@ -205,14 +205,14 @@ static int ospf_bfd_interface_dest_update(ZAPI_CALLBACK_ARGS)
        struct ospf_neighbor *nbr = NULL;
        struct route_node *node;
        struct route_node *n_node;
-       struct prefix p;
+       struct prefix p, src_p;
        int status;
        int old_status;
        struct bfd_info *bfd_info;
        struct timeval tv;
 
-       ifp = bfd_get_peer_info(zclient->ibuf, &p, NULL, &status,
-                               NULL, vrf_id);
+       ifp = bfd_get_peer_info(zclient->ibuf, &p, &src_p, &status, NULL,
+                               vrf_id);
 
        if ((ifp == NULL) || (p.family != AF_INET))
                return 0;

diff --git a/pimd/pim_bfd.c b/pimd/pim_bfd.c
index 146b53fa8f7fdbd98ca643c76d2a9e0a97357fd9..a103fc5f8ddfef9aba30a3120a81cd7392393746 100644 (file)
--- a/pimd/pim_bfd.c
+++ b/pimd/pim_bfd.c
@@ -217,7 +217,7 @@ static int pim_bfd_interface_dest_update(ZAPI_CALLBACK_ARGS)
 {
        struct interface *ifp = NULL;
        struct pim_interface *pim_ifp = NULL;
-       struct prefix p;
+       struct prefix p, src_p;
        int status;
        char msg[100];
        int old_status;
@@ -227,8 +227,8 @@ static int pim_bfd_interface_dest_update(ZAPI_CALLBACK_ARGS)
        struct listnode *neigh_nextnode = NULL;
        struct pim_neighbor *neigh = NULL;
 
-       ifp = bfd_get_peer_info(zclient->ibuf, &p, NULL, &status,
-                               NULL, vrf_id);
+       ifp = bfd_get_peer_info(zclient->ibuf, &p, &src_p, &status, NULL,
+                               vrf_id);
 
        if ((ifp == NULL) || (p.family != AF_INET))
                return 0;