[quagga-dev,15436] zebra: fix a crash in static_add_ipv6 caused by a NULL dereference

Message ID 1465240224-26278-1-git-send-email-chris@opensourcerouting.org
State Under Review
Headers show

Commit Message

Christian Franke June 6, 2016, 7:10 p.m.
From: Christian Franke <nobody@nowhere.ws>

si will be NULL after end of the preceeding for loop. update is the
right static route info to use for deleting the old route.

Signed-off-by: Christian Franke <chris@opensourcerouting.org>
---

Also, the reference counting for static routes seems to be broken.
It seems to me like:
 a) static_add_ipv6 will increment the route node reference counter
    in stable for every call that updates the distance of a route
    (route_node_get without any unlock node)
 b) static_delete_ipv6 will increment the same reference counter when a
    route is deleted (route_node_lookup, but unlock only if nothing to
    delete)
 c) Both preceeding issues seem to be present for IPv4 too.

I am not yet 100% sure how the static route reference counting is
supposed to work. Should the route nodes have a reference if the list
of static routes is present? Or one for every route in that list?
Is anybody on this list familiar with this part of the code?

 zebra/zebra_rib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

cisystem@netdef.org June 6, 2016, 8:20 p.m. | #1
Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF CI System <cisystem@netdef.org>

This is an EXPERIMENTAL automated CI system.
For questions and feedback, feel free to email
Martin Winter <mwinter@opensourcerouting.org>.

Patches applied :
  Patchwork 1955: http://patchwork.quagga.net/patch/1955
       [quagga-dev,15436] zebra: fix a crash in static_add_ipv6 caused by a NULL dereference
Tested on top of Git : 86c5d2e (as of 20160315.231717 UTC)
CI System Testrun URL: https://ci1.netdef.org/browse/QUAGGA-QPWORK-296/


Regards,
  NetDEF/OpenSourceRouting Continous Integration (CI) System

---
OpenSourceRouting.org is a project of the Network Device Education Foundation,
For more information, see www.netdef.org and www.opensourcerouting.org
For questions in regards to this CI System, contact Martin Winter, mwinter@netdef.org
Donald Sharp June 7, 2016, 1:33 a.m. | #2
Acked-by: Donald Sharp <sharpd@cumulusnetworks.com>

I agree with you, it's not clear to me how the static route reference
counting is supposed to work.

donald

On Mon, Jun 6, 2016 at 3:10 PM, Christian Franke <
chris@opensourcerouting.org> wrote:

> From: Christian Franke <nobody@nowhere.ws>
>
> si will be NULL after end of the preceeding for loop. update is the
> right static route info to use for deleting the old route.
>
> Signed-off-by: Christian Franke <chris@opensourcerouting.org>
> ---
>
> Also, the reference counting for static routes seems to be broken.
> It seems to me like:
>  a) static_add_ipv6 will increment the route node reference counter
>     in stable for every call that updates the distance of a route
>     (route_node_get without any unlock node)
>  b) static_delete_ipv6 will increment the same reference counter when a
>     route is deleted (route_node_lookup, but unlock only if nothing to
>     delete)
>  c) Both preceeding issues seem to be present for IPv4 too.
>
> I am not yet 100% sure how the static route reference counting is
> supposed to work. Should the route nodes have a reference if the list
> of static routes is present? Or one for every route in that list?
> Is anybody on this list familiar with this part of the code?
>
>  zebra/zebra_rib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
> index 1650dab..f4df119 100644
> --- a/zebra/zebra_rib.c
> +++ b/zebra/zebra_rib.c
> @@ -2841,7 +2841,7 @@ static_add_ipv6 (struct prefix *p, u_char type,
> struct in6_addr *gate,
>      }
>
>    if (update)
> -    static_delete_ipv6(p, type, gate, ifname, si->distance, vrf_id);
> +    static_delete_ipv6(p, type, gate, ifname, update->distance, vrf_id);
>
>    /* Make new static route structure. */
>    si = XCALLOC (MTYPE_STATIC_ROUTE, sizeof (struct static_route));
> --
> 2.8.0
>
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>

Patch hide | download patch | download mbox

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index 1650dab..f4df119 100644
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -2841,7 +2841,7 @@  static_add_ipv6 (struct prefix *p, u_char type, struct in6_addr *gate,
     }
 
   if (update)
-    static_delete_ipv6(p, type, gate, ifname, si->distance, vrf_id);
+    static_delete_ipv6(p, type, gate, ifname, update->distance, vrf_id);
 
   /* Make new static route structure. */
   si = XCALLOC (MTYPE_STATIC_ROUTE, sizeof (struct static_route));