[cmaster-next] [PATCH] bgpd: fix invalid memory access in peer_free()
We shoult not call bgp_unlock() before calling bgp_delete_connected_nexthop() in the peer_free() function. Otherwise, if bgp->lock reaches zero, bgp_free() is called and peer->bgp becomes an invalid pointer in the bgp_delete_connected_nexthop() function. To fix this, move the call to bgp_unlock() to the end of peer_free(). Bug exposed by commit 37d361e ("bgpd: plug several memleaks"). Signed-off-by: Renato Westphal <renato@opensourcerouting.org> --- bgpd/bgpd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 22d4dd8..d5aff84 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1019,8 +1019,6 @@ peer_free (struct peer *peer) { assert (peer->status == Deleted); - bgp_unlock(peer->bgp); - /* this /ought/ to have been done already through bgp_stop earlier, * but just to be sure.. */ @@ -1092,6 +1090,8 @@ peer_free (struct peer *peer) bfd_info_free(&(peer->bfd_info)); + bgp_unlock(peer->bgp); + memset (peer, 0, sizeof (struct peer)); XFREE (MTYPE_BGP_PEER, peer); -- 1.9.1
I've applied it on stable/2.0 even though we don't have the "plug memleaks" patch there. (Maybe we should?) -David On Mon, Nov 28, 2016 at 04:47:13PM -0200, Renato Westphal wrote:
We shoult not call bgp_unlock() before calling bgp_delete_connected_nexthop() in the peer_free() function. Otherwise, if bgp->lock reaches zero, bgp_free() is called and peer->bgp becomes an invalid pointer in the bgp_delete_connected_nexthop() function.
To fix this, move the call to bgp_unlock() to the end of peer_free().
Bug exposed by commit 37d361e ("bgpd: plug several memleaks").
Signed-off-by: Renato Westphal <renato@opensourcerouting.org> --- bgpd/bgpd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 22d4dd8..d5aff84 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1019,8 +1019,6 @@ peer_free (struct peer *peer) { assert (peer->status == Deleted);
- bgp_unlock(peer->bgp); - /* this /ought/ to have been done already through bgp_stop earlier, * but just to be sure.. */ @@ -1092,6 +1090,8 @@ peer_free (struct peer *peer)
bfd_info_free(&(peer->bfd_info));
+ bgp_unlock(peer->bgp); + memset (peer, 0, sizeof (struct peer));
XFREE (MTYPE_BGP_PEER, peer); -- 1.9.1
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
I think memory leaks fixing is good for a release candidate. donald On Thu, Dec 1, 2016 at 10:38 AM, David Lamparter <david@opensourcerouting.org> wrote:
I've applied it on stable/2.0 even though we don't have the "plug memleaks" patch there. (Maybe we should?)
-David
On Mon, Nov 28, 2016 at 04:47:13PM -0200, Renato Westphal wrote:
We shoult not call bgp_unlock() before calling bgp_delete_connected_nexthop() in the peer_free() function. Otherwise, if bgp->lock reaches zero, bgp_free() is called and peer->bgp becomes an invalid pointer in the bgp_delete_connected_nexthop() function.
To fix this, move the call to bgp_unlock() to the end of peer_free().
Bug exposed by commit 37d361e ("bgpd: plug several memleaks").
Signed-off-by: Renato Westphal <renato@opensourcerouting.org> --- bgpd/bgpd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 22d4dd8..d5aff84 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1019,8 +1019,6 @@ peer_free (struct peer *peer) { assert (peer->status == Deleted);
- bgp_unlock(peer->bgp); - /* this /ought/ to have been done already through bgp_stop earlier, * but just to be sure.. */ @@ -1092,6 +1090,8 @@ peer_free (struct peer *peer)
bfd_info_free(&(peer->bfd_info));
+ bgp_unlock(peer->bgp); + memset (peer, 0, sizeof (struct peer));
XFREE (MTYPE_BGP_PEER, peer); -- 1.9.1
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
Agreed, I think cmaster-next-renato is ready to be merged. Martin did a full ANVL run on this branch and the results look good now: https://drive.google.com/drive/folders/0B8W_T0dxQfwxdXo4STRKTG9SR0U?usp=shar... On Thu, Dec 1, 2016 at 1:47 PM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
I think memory leaks fixing is good for a release candidate.
donald
On Thu, Dec 1, 2016 at 10:38 AM, David Lamparter <david@opensourcerouting.org> wrote:
I've applied it on stable/2.0 even though we don't have the "plug memleaks" patch there. (Maybe we should?)
-David
On Mon, Nov 28, 2016 at 04:47:13PM -0200, Renato Westphal wrote:
We shoult not call bgp_unlock() before calling bgp_delete_connected_nexthop() in the peer_free() function. Otherwise, if bgp->lock reaches zero, bgp_free() is called and peer->bgp becomes an invalid pointer in the bgp_delete_connected_nexthop() function.
To fix this, move the call to bgp_unlock() to the end of peer_free().
Bug exposed by commit 37d361e ("bgpd: plug several memleaks").
Signed-off-by: Renato Westphal <renato@opensourcerouting.org> --- bgpd/bgpd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 22d4dd8..d5aff84 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1019,8 +1019,6 @@ peer_free (struct peer *peer) { assert (peer->status == Deleted);
- bgp_unlock(peer->bgp); - /* this /ought/ to have been done already through bgp_stop earlier, * but just to be sure.. */ @@ -1092,6 +1090,8 @@ peer_free (struct peer *peer)
bfd_info_free(&(peer->bfd_info));
+ bgp_unlock(peer->bgp); + memset (peer, 0, sizeof (struct peer));
XFREE (MTYPE_BGP_PEER, peer); -- 1.9.1
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next
-- Renato Westphal
participants (3)
-
David Lamparter -
Donald Sharp -
Renato Westphal