[quagga-dev,15561,07/16] bgpd: fix memory leaks in show commands

Message ID 1465927630-27768-7-git-send-email-chris@opensourcerouting.org
State Under Review
Headers show
Series
  • [quagga-dev,15552,01/16] zebra: fix null pointer dereference in zsend_ipv4_nexthop_lookup_mrib
Related show

Commit Message

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

sockunion_str2su allocates a struct sockunion that used to be leaked
in the show commands. Use str2sockunion and keep the information
on the stack instead.

Signed-off-by: Christian Franke <chris@opensourcerouting.org>
---
 bgpd/bgp_encap.c   | 38 +++++++++++++++++---------------------
 bgpd/bgp_mplsvpn.c | 18 ++++++++----------
 bgpd/bgp_route.c   | 10 ++++++----
 3 files changed, 31 insertions(+), 35 deletions(-)

Comments

Donald Sharp June 15, 2016, 12:58 p.m. | #1
Acked-by: Donald Sharp <sharpd@cumulusnetworks.com>

On Tue, Jun 14, 2016 at 2:07 PM, Christian Franke <
chris@opensourcerouting.org> wrote:

> From: Christian Franke <nobody@nowhere.ws>
>
> sockunion_str2su allocates a struct sockunion that used to be leaked
> in the show commands. Use str2sockunion and keep the information
> on the stack instead.
>
> Signed-off-by: Christian Franke <chris@opensourcerouting.org>
> ---
>  bgpd/bgp_encap.c   | 38 +++++++++++++++++---------------------
>  bgpd/bgp_mplsvpn.c | 18 ++++++++----------
>  bgpd/bgp_route.c   | 10 ++++++----
>  3 files changed, 31 insertions(+), 35 deletions(-)
>
> diff --git a/bgpd/bgp_encap.c b/bgpd/bgp_encap.c
> index 1a09ba6..73ab8958 100644
> --- a/bgpd/bgp_encap.c
> +++ b/bgpd/bgp_encap.c
> @@ -649,24 +649,23 @@ DEFUN (show_bgp_ipv4_encap_neighbor_routes,
>         "Neighbor to display information about\n"
>         "Display routes learned from neighbor\n")
>  {
> -  union sockunion *su;
> +  union sockunion su;
>    struct peer *peer;
> -
> -  su = sockunion_str2su (argv[0]);
> -  if (su == NULL)
> +
> +  if (str2sockunion(argv[0], &su))
>      {
>        vty_out (vty, "Malformed address: %s%s", argv[0], VTY_NEWLINE);
>                 return CMD_WARNING;
>      }
>
> -  peer = peer_lookup (NULL, su);
> +  peer = peer_lookup (NULL, &su);
>    if (! peer || ! peer->afc[AFI_IP][SAFI_ENCAP])
>      {
>        vty_out (vty, "%% No such neighbor or address family%s",
> VTY_NEWLINE);
>        return CMD_WARNING;
>      }
>
> -  return bgp_show_encap (vty, AFI_IP, NULL, bgp_show_type_neighbor, su,
> 0);
> +  return bgp_show_encap (vty, AFI_IP, NULL, bgp_show_type_neighbor, &su,
> 0);
>  }
>
>  DEFUN (show_bgp_ipv6_encap_neighbor_routes,
> @@ -680,24 +679,23 @@ DEFUN (show_bgp_ipv6_encap_neighbor_routes,
>         "Neighbor to display information about\n"
>         "Display routes learned from neighbor\n")
>  {
> -  union sockunion *su;
> +  union sockunion su;
>    struct peer *peer;
>
> -  su = sockunion_str2su (argv[0]);
> -  if (su == NULL)
> +  if (str2sockunion(argv[0], &su))
>      {
>        vty_out (vty, "Malformed address: %s%s", argv[0], VTY_NEWLINE);
>                 return CMD_WARNING;
>      }
>
> -  peer = peer_lookup (NULL, su);
> +  peer = peer_lookup (NULL, &su);
>    if (! peer || ! peer->afc[AFI_IP6][SAFI_ENCAP])
>      {
>        vty_out (vty, "%% No such neighbor or address family%s",
> VTY_NEWLINE);
>        return CMD_WARNING;
>      }
>
> -  return bgp_show_encap (vty, AFI_IP6, NULL, bgp_show_type_neighbor, su,
> 0);
> +  return bgp_show_encap (vty, AFI_IP6, NULL, bgp_show_type_neighbor, &su,
> 0);
>  }
>
>  DEFUN (show_bgp_ipv4_encap_rd_neighbor_routes,
> @@ -715,7 +713,7 @@ DEFUN (show_bgp_ipv4_encap_rd_neighbor_routes,
>         "Display routes learned from neighbor\n")
>  {
>    int ret;
> -  union sockunion *su;
> +  union sockunion su;
>    struct peer *peer;
>    struct prefix_rd prd;
>
> @@ -726,21 +724,20 @@ DEFUN (show_bgp_ipv4_encap_rd_neighbor_routes,
>        return CMD_WARNING;
>      }
>
> -  su = sockunion_str2su (argv[1]);
> -  if (su == NULL)
> +  if (str2sockunion(argv[1], &su))
>      {
>        vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
>                 return CMD_WARNING;
>      }
>
> -  peer = peer_lookup (NULL, su);
> +  peer = peer_lookup (NULL, &su);
>    if (! peer || ! peer->afc[AFI_IP][SAFI_ENCAP])
>      {
>        vty_out (vty, "%% No such neighbor or address family%s",
> VTY_NEWLINE);
>        return CMD_WARNING;
>      }
>
> -  return bgp_show_encap (vty, AFI_IP, &prd, bgp_show_type_neighbor, su,
> 0);
> +  return bgp_show_encap (vty, AFI_IP, &prd, bgp_show_type_neighbor, &su,
> 0);
>  }
>
>  DEFUN (show_bgp_ipv6_encap_rd_neighbor_routes,
> @@ -758,7 +755,7 @@ DEFUN (show_bgp_ipv6_encap_rd_neighbor_routes,
>         "Display routes learned from neighbor\n")
>  {
>    int ret;
> -  union sockunion *su;
> +  union sockunion su;
>    struct peer *peer;
>    struct prefix_rd prd;
>
> @@ -769,21 +766,20 @@ DEFUN (show_bgp_ipv6_encap_rd_neighbor_routes,
>        return CMD_WARNING;
>      }
>
> -  su = sockunion_str2su (argv[1]);
> -  if (su == NULL)
> +  if (str2sockunion(argv[1], &su))
>      {
>        vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
>                 return CMD_WARNING;
>      }
>
> -  peer = peer_lookup (NULL, su);
> +  peer = peer_lookup (NULL, &su);
>    if (! peer || ! peer->afc[AFI_IP6][SAFI_ENCAP])
>      {
>        vty_out (vty, "%% No such neighbor or address family%s",
> VTY_NEWLINE);
>        return CMD_WARNING;
>      }
>
> -  return bgp_show_encap (vty, AFI_IP6, &prd, bgp_show_type_neighbor, su,
> 0);
> +  return bgp_show_encap (vty, AFI_IP6, &prd, bgp_show_type_neighbor, &su,
> 0);
>  }
>
>  DEFUN (show_bgp_ipv4_encap_neighbor_advertised_routes,
> diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c
> index 08a4272..93406f3 100644
> --- a/bgpd/bgp_mplsvpn.c
> +++ b/bgpd/bgp_mplsvpn.c
> @@ -969,7 +969,7 @@ DEFUN (show_bgp_ipv4_vpn_rd_neighbor_routes,
>         "Display routes learned from neighbor\n")
>  {
>    int ret;
> -  union sockunion *su;
> +  union sockunion su;
>    struct peer *peer;
>    struct prefix_rd prd;
>
> @@ -980,21 +980,20 @@ DEFUN (show_bgp_ipv4_vpn_rd_neighbor_routes,
>        return CMD_WARNING;
>      }
>
> -  su = sockunion_str2su (argv[1]);
> -  if (su == NULL)
> +  if (str2sockunion(argv[1], &su))
>      {
>        vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
>                 return CMD_WARNING;
>      }
>
> -  peer = peer_lookup (NULL, su);
> +  peer = peer_lookup (NULL, &su);
>    if (! peer || ! peer->afc[AFI_IP][SAFI_MPLS_VPN])
>      {
>        vty_out (vty, "%% No such neighbor or address family%s",
> VTY_NEWLINE);
>        return CMD_WARNING;
>      }
>
> -  return bgp_show_mpls_vpn (vty, AFI_IP, &prd, bgp_show_type_neighbor,
> su, 0);
> +  return bgp_show_mpls_vpn (vty, AFI_IP, &prd, bgp_show_type_neighbor,
> &su, 0);
>  }
>  DEFUN (show_bgp_ipv6_vpn_rd_neighbor_routes,
>         show_bgp_ipv6_vpn_rd_neighbor_routes_cmd,
> @@ -1010,7 +1009,7 @@ DEFUN (show_bgp_ipv6_vpn_rd_neighbor_routes,
>         "Display routes learned from neighbor\n")
>  {
>    int ret;
> -  union sockunion *su;
> +  union sockunion su;
>    struct peer *peer;
>    struct prefix_rd prd;
>
> @@ -1021,21 +1020,20 @@ DEFUN (show_bgp_ipv6_vpn_rd_neighbor_routes,
>        return CMD_WARNING;
>      }
>
> -  su = sockunion_str2su (argv[1]);
> -  if (su == NULL)
> +  if (str2sockunion(argv[1], &su))
>      {
>        vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
>                 return CMD_WARNING;
>      }
>
> -  peer = peer_lookup (NULL, su);
> +  peer = peer_lookup (NULL, &su);
>    if (! peer || ! peer->afc[AFI_IP6][SAFI_MPLS_VPN])
>      {
>        vty_out (vty, "%% No such neighbor or address family%s",
> VTY_NEWLINE);
>        return CMD_WARNING;
>      }
>
> -  return bgp_show_mpls_vpn (vty, AFI_IP6, &prd, bgp_show_type_neighbor,
> su, 0);
> +  return bgp_show_mpls_vpn (vty, AFI_IP6, &prd, bgp_show_type_neighbor,
> &su, 0);
>  }
>
>  void
> diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
> index 2fd1675..4cd2271 100644
> --- a/bgpd/bgp_route.c
> +++ b/bgpd/bgp_route.c
> @@ -9085,7 +9085,7 @@ bgp_show_community (struct vty *vty, const char
> *view_name, int argc,
>    struct community *com;
>    struct buffer *b;
>    struct bgp *bgp;
> -  int i;
> +  int i, rv;
>    char *str;
>    int first = 0;
>
> @@ -9136,9 +9136,11 @@ bgp_show_community (struct vty *vty, const char
> *view_name, int argc,
>        return CMD_WARNING;
>      }
>
> -  return bgp_show (vty, bgp, afi, safi,
> -                   (exact ? bgp_show_type_community_exact :
> -                           bgp_show_type_community), com);
> +  rv = bgp_show (vty, bgp, afi, safi,
> +                 (exact ? bgp_show_type_community_exact :
> +                         bgp_show_type_community), com);
> +  community_free(com);
> +  return rv;
>  }
>
>  DEFUN (show_ip_bgp_community,
> --
> 2.8.0
>
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> https://lists.quagga.net/mailman/listinfo/quagga-dev
>
Philippe Guibert June 15, 2016, 3:50 p.m. | #2
On Tue, Jun 14, 2016 at 8:07 PM, Christian Franke <
chris@opensourcerouting.org> wrote:

Hello Christian,

+  rv = bgp_show (vty, bgp, afi, safi,
> +                 (exact ? bgp_show_type_community_exact :
> +                         bgp_show_type_community), com);
> +  community_free(com);
>

The usage of the new API is safer.
If the quagga testing were not using sockunion_str2su(), this command could
have been removed.
It is possible to mention in the commit log the memory leak related to
community structure ?

Thanks,
Regards,

Philippe
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Vincent JARDIN June 15, 2016, 10:31 p.m. | #3
Philippe,

please, don't post HTML, it does not bring values to the archives:
   https://lists.quagga.net/pipermail/quagga-dev/2016-June/015678.html

An HTML attachment was scrubbed...
URL: 
<http://lists.quagga.net/pipermail/quagga-dev/attachments/20160615/596cb1fc/attachment-0001.html>

Thank you,
   Vincent
Philippe Guibert June 21, 2016, 9:35 a.m. | #4
On Wed, Jun 15, 2016 at 5:50 PM, Philippe Guibert <
philippe.guibert@6wind.com> wrote:

Hello,

Is this patch that I did ack't applied?

Regards,

Philippe
Vincent JARDIN June 21, 2016, 9:54 a.m. | #5
> Hello,
>
> Is this patch that I did ack't applied?
>
> Regards,
> Philippe

Please, Philippe, again, no HTML email (even in 2016 :D), otherwise it 
is hardly indexed:
https://lists.quagga.net/pipermail/quagga-dev/2016-June/015752.html

Patch hide | download patch | download mbox

diff --git a/bgpd/bgp_encap.c b/bgpd/bgp_encap.c
index 1a09ba6..73ab8958 100644
--- a/bgpd/bgp_encap.c
+++ b/bgpd/bgp_encap.c
@@ -649,24 +649,23 @@  DEFUN (show_bgp_ipv4_encap_neighbor_routes,
        "Neighbor to display information about\n"
        "Display routes learned from neighbor\n")
 {
-  union sockunion *su;
+  union sockunion su;
   struct peer *peer;
-  
-  su = sockunion_str2su (argv[0]);
-  if (su == NULL)
+
+  if (str2sockunion(argv[0], &su))
     {
       vty_out (vty, "Malformed address: %s%s", argv[0], VTY_NEWLINE);
                return CMD_WARNING;
     }
 
-  peer = peer_lookup (NULL, su);
+  peer = peer_lookup (NULL, &su);
   if (! peer || ! peer->afc[AFI_IP][SAFI_ENCAP])
     {
       vty_out (vty, "%% No such neighbor or address family%s", VTY_NEWLINE);
       return CMD_WARNING;
     }
 
-  return bgp_show_encap (vty, AFI_IP, NULL, bgp_show_type_neighbor, su, 0);
+  return bgp_show_encap (vty, AFI_IP, NULL, bgp_show_type_neighbor, &su, 0);
 }
 
 DEFUN (show_bgp_ipv6_encap_neighbor_routes,
@@ -680,24 +679,23 @@  DEFUN (show_bgp_ipv6_encap_neighbor_routes,
        "Neighbor to display information about\n"
        "Display routes learned from neighbor\n")
 {
-  union sockunion *su;
+  union sockunion su;
   struct peer *peer;
   
-  su = sockunion_str2su (argv[0]);
-  if (su == NULL)
+  if (str2sockunion(argv[0], &su))
     {
       vty_out (vty, "Malformed address: %s%s", argv[0], VTY_NEWLINE);
                return CMD_WARNING;
     }
 
-  peer = peer_lookup (NULL, su);
+  peer = peer_lookup (NULL, &su);
   if (! peer || ! peer->afc[AFI_IP6][SAFI_ENCAP])
     {
       vty_out (vty, "%% No such neighbor or address family%s", VTY_NEWLINE);
       return CMD_WARNING;
     }
 
-  return bgp_show_encap (vty, AFI_IP6, NULL, bgp_show_type_neighbor, su, 0);
+  return bgp_show_encap (vty, AFI_IP6, NULL, bgp_show_type_neighbor, &su, 0);
 }
 
 DEFUN (show_bgp_ipv4_encap_rd_neighbor_routes,
@@ -715,7 +713,7 @@  DEFUN (show_bgp_ipv4_encap_rd_neighbor_routes,
        "Display routes learned from neighbor\n")
 {
   int ret;
-  union sockunion *su;
+  union sockunion su;
   struct peer *peer;
   struct prefix_rd prd;
 
@@ -726,21 +724,20 @@  DEFUN (show_bgp_ipv4_encap_rd_neighbor_routes,
       return CMD_WARNING;
     }
 
-  su = sockunion_str2su (argv[1]);
-  if (su == NULL)
+  if (str2sockunion(argv[1], &su))
     {
       vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
                return CMD_WARNING;
     }
 
-  peer = peer_lookup (NULL, su);
+  peer = peer_lookup (NULL, &su);
   if (! peer || ! peer->afc[AFI_IP][SAFI_ENCAP])
     {
       vty_out (vty, "%% No such neighbor or address family%s", VTY_NEWLINE);
       return CMD_WARNING;
     }
 
-  return bgp_show_encap (vty, AFI_IP, &prd, bgp_show_type_neighbor, su, 0);
+  return bgp_show_encap (vty, AFI_IP, &prd, bgp_show_type_neighbor, &su, 0);
 }
 
 DEFUN (show_bgp_ipv6_encap_rd_neighbor_routes,
@@ -758,7 +755,7 @@  DEFUN (show_bgp_ipv6_encap_rd_neighbor_routes,
        "Display routes learned from neighbor\n")
 {
   int ret;
-  union sockunion *su;
+  union sockunion su;
   struct peer *peer;
   struct prefix_rd prd;
 
@@ -769,21 +766,20 @@  DEFUN (show_bgp_ipv6_encap_rd_neighbor_routes,
       return CMD_WARNING;
     }
 
-  su = sockunion_str2su (argv[1]);
-  if (su == NULL)
+  if (str2sockunion(argv[1], &su))
     {
       vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
                return CMD_WARNING;
     }
 
-  peer = peer_lookup (NULL, su);
+  peer = peer_lookup (NULL, &su);
   if (! peer || ! peer->afc[AFI_IP6][SAFI_ENCAP])
     {
       vty_out (vty, "%% No such neighbor or address family%s", VTY_NEWLINE);
       return CMD_WARNING;
     }
 
-  return bgp_show_encap (vty, AFI_IP6, &prd, bgp_show_type_neighbor, su, 0);
+  return bgp_show_encap (vty, AFI_IP6, &prd, bgp_show_type_neighbor, &su, 0);
 }
 
 DEFUN (show_bgp_ipv4_encap_neighbor_advertised_routes,
diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c
index 08a4272..93406f3 100644
--- a/bgpd/bgp_mplsvpn.c
+++ b/bgpd/bgp_mplsvpn.c
@@ -969,7 +969,7 @@  DEFUN (show_bgp_ipv4_vpn_rd_neighbor_routes,
        "Display routes learned from neighbor\n")
 {
   int ret;
-  union sockunion *su;
+  union sockunion su;
   struct peer *peer;
   struct prefix_rd prd;
 
@@ -980,21 +980,20 @@  DEFUN (show_bgp_ipv4_vpn_rd_neighbor_routes,
       return CMD_WARNING;
     }
 
-  su = sockunion_str2su (argv[1]);
-  if (su == NULL)
+  if (str2sockunion(argv[1], &su))
     {
       vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
                return CMD_WARNING;
     }
 
-  peer = peer_lookup (NULL, su);
+  peer = peer_lookup (NULL, &su);
   if (! peer || ! peer->afc[AFI_IP][SAFI_MPLS_VPN])
     {
       vty_out (vty, "%% No such neighbor or address family%s", VTY_NEWLINE);
       return CMD_WARNING;
     }
 
-  return bgp_show_mpls_vpn (vty, AFI_IP, &prd, bgp_show_type_neighbor, su, 0);
+  return bgp_show_mpls_vpn (vty, AFI_IP, &prd, bgp_show_type_neighbor, &su, 0);
 }
 DEFUN (show_bgp_ipv6_vpn_rd_neighbor_routes,
        show_bgp_ipv6_vpn_rd_neighbor_routes_cmd,
@@ -1010,7 +1009,7 @@  DEFUN (show_bgp_ipv6_vpn_rd_neighbor_routes,
        "Display routes learned from neighbor\n")
 {
   int ret;
-  union sockunion *su;
+  union sockunion su;
   struct peer *peer;
   struct prefix_rd prd;
 
@@ -1021,21 +1020,20 @@  DEFUN (show_bgp_ipv6_vpn_rd_neighbor_routes,
       return CMD_WARNING;
     }
 
-  su = sockunion_str2su (argv[1]);
-  if (su == NULL)
+  if (str2sockunion(argv[1], &su))
     {
       vty_out (vty, "Malformed address: %s%s", argv[1], VTY_NEWLINE);
                return CMD_WARNING;
     }
 
-  peer = peer_lookup (NULL, su);
+  peer = peer_lookup (NULL, &su);
   if (! peer || ! peer->afc[AFI_IP6][SAFI_MPLS_VPN])
     {
       vty_out (vty, "%% No such neighbor or address family%s", VTY_NEWLINE);
       return CMD_WARNING;
     }
 
-  return bgp_show_mpls_vpn (vty, AFI_IP6, &prd, bgp_show_type_neighbor, su, 0);
+  return bgp_show_mpls_vpn (vty, AFI_IP6, &prd, bgp_show_type_neighbor, &su, 0);
 }
 
 void
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 2fd1675..4cd2271 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -9085,7 +9085,7 @@  bgp_show_community (struct vty *vty, const char *view_name, int argc,
   struct community *com;
   struct buffer *b;
   struct bgp *bgp;
-  int i;
+  int i, rv;
   char *str;
   int first = 0;
 
@@ -9136,9 +9136,11 @@  bgp_show_community (struct vty *vty, const char *view_name, int argc,
       return CMD_WARNING;
     }
 
-  return bgp_show (vty, bgp, afi, safi,
-                   (exact ? bgp_show_type_community_exact :
-		            bgp_show_type_community), com);
+  rv = bgp_show (vty, bgp, afi, safi,
+                 (exact ? bgp_show_type_community_exact :
+		          bgp_show_type_community), com);
+  community_free(com);
+  return rv;
 }
 
 DEFUN (show_ip_bgp_community,