Re: ripd: memory leak and double free/use-after-free
On Wed, Dec 11, 2019 at 10:11:58AM +0100, Sebastian Benoit wrote:
> Remi Locherer(remi.loche...@relo.ch) on 2019.12.10 22:39:32 +0100:
> > On Tue, Dec 10, 2019 at 07:05:27PM +0100, Hiltjo Posthuma wrote:
> > > Hi,
> > >
> > > While looking at the code of ripd:
> > >
> > > I think there are (also) 2 small memleaks in a debug/error path
> > > (IMSG_REQUEST_ADD and IMSG_RESPONSE_ADD). It breaks out before adding the
> > > struct rip_route as an entry by the add_entry function (which adds it and
> > > adds
> > > a reference count) in the log_debug block.
> > >
> > > clang-analyzer also pointed at a double-free and use of free'd data: the
> > > function kroute_insert frees kr and returns -1 when struct kroute is
> > > duplicate.
> > >
> > > Patch below (untested):
> > >
> >
> > OK remi@
>
> go ahead and commit it, ok benno@
Thank you for the patch! I just committed it.
Remi
>
> >
> > >
> > > diff --git a/usr.sbin/ripd/kroute.c b/usr.sbin/ripd/kroute.c
> > > index 6e7449e0909..71758a75e44 100644
> > > --- a/usr.sbin/ripd/kroute.c
> > > +++ b/usr.sbin/ripd/kroute.c
> > > @@ -183,8 +183,7 @@ kr_change_fib(struct kroute_node *kr, struct kroute
> > > *kroute, int action)
> > >
> > > if (kroute_insert(kr) == -1) {
> > > log_debug("kr_update_fib: cannot insert %s",
> > > - inet_ntoa(kr->r.nexthop));
> > > - free(kr);
> > > + inet_ntoa(kroute->nexthop));
> > > }
> > > } else
> > > kr->r.nexthop.s_addr = kroute->nexthop.s_addr;
> > > diff --git a/usr.sbin/ripd/ripe.c b/usr.sbin/ripd/ripe.c
> > > index d83901e245f..1f6f9b6583f 100644
> > > --- a/usr.sbin/ripd/ripe.c
> > > +++ b/usr.sbin/ripd/ripe.c
> > > @@ -351,6 +351,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
> > > NULL) {
> > > log_debug("unknown neighbor id %u",
> > > imsg.hdr.peerid);
> > > + free(rr);
> > > break;
> > > }
> > > add_entry(>rq_list, rr);
> > > @@ -396,6 +397,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
> > > if ((nbr = nbr_find_peerid(imsg.hdr.peerid)) == NULL) {
> > > log_debug("unknown neighbor id %u",
> > > imsg.hdr.peerid);
> > > + free(rr);
> > > break;
> > > }
> > > iface = nbr->iface;
> > >
> > > --
> > > Kind regards,
> > > Hiltjo
> > >
> >
>
Re: ripd: memory leak and double free/use-after-free
Remi Locherer(remi.loche...@relo.ch) on 2019.12.10 22:39:32 +0100:
> On Tue, Dec 10, 2019 at 07:05:27PM +0100, Hiltjo Posthuma wrote:
> > Hi,
> >
> > While looking at the code of ripd:
> >
> > I think there are (also) 2 small memleaks in a debug/error path
> > (IMSG_REQUEST_ADD and IMSG_RESPONSE_ADD). It breaks out before adding the
> > struct rip_route as an entry by the add_entry function (which adds it and
> > adds
> > a reference count) in the log_debug block.
> >
> > clang-analyzer also pointed at a double-free and use of free'd data: the
> > function kroute_insert frees kr and returns -1 when struct kroute is
> > duplicate.
> >
> > Patch below (untested):
> >
>
> OK remi@
go ahead and commit it, ok benno@
>
> >
> > diff --git a/usr.sbin/ripd/kroute.c b/usr.sbin/ripd/kroute.c
> > index 6e7449e0909..71758a75e44 100644
> > --- a/usr.sbin/ripd/kroute.c
> > +++ b/usr.sbin/ripd/kroute.c
> > @@ -183,8 +183,7 @@ kr_change_fib(struct kroute_node *kr, struct kroute
> > *kroute, int action)
> >
> > if (kroute_insert(kr) == -1) {
> > log_debug("kr_update_fib: cannot insert %s",
> > - inet_ntoa(kr->r.nexthop));
> > - free(kr);
> > + inet_ntoa(kroute->nexthop));
> > }
> > } else
> > kr->r.nexthop.s_addr = kroute->nexthop.s_addr;
> > diff --git a/usr.sbin/ripd/ripe.c b/usr.sbin/ripd/ripe.c
> > index d83901e245f..1f6f9b6583f 100644
> > --- a/usr.sbin/ripd/ripe.c
> > +++ b/usr.sbin/ripd/ripe.c
> > @@ -351,6 +351,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
> > NULL) {
> > log_debug("unknown neighbor id %u",
> > imsg.hdr.peerid);
> > + free(rr);
> > break;
> > }
> > add_entry(>rq_list, rr);
> > @@ -396,6 +397,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
> > if ((nbr = nbr_find_peerid(imsg.hdr.peerid)) == NULL) {
> > log_debug("unknown neighbor id %u",
> > imsg.hdr.peerid);
> > + free(rr);
> > break;
> > }
> > iface = nbr->iface;
> >
> > --
> > Kind regards,
> > Hiltjo
> >
>
Re: ripd: memory leak and double free/use-after-free
On Tue, Dec 10, 2019 at 07:05:27PM +0100, Hiltjo Posthuma wrote:
> Hi,
>
> While looking at the code of ripd:
>
> I think there are (also) 2 small memleaks in a debug/error path
> (IMSG_REQUEST_ADD and IMSG_RESPONSE_ADD). It breaks out before adding the
> struct rip_route as an entry by the add_entry function (which adds it and adds
> a reference count) in the log_debug block.
>
> clang-analyzer also pointed at a double-free and use of free'd data: the
> function kroute_insert frees kr and returns -1 when struct kroute is
> duplicate.
>
> Patch below (untested):
>
OK remi@
>
> diff --git a/usr.sbin/ripd/kroute.c b/usr.sbin/ripd/kroute.c
> index 6e7449e0909..71758a75e44 100644
> --- a/usr.sbin/ripd/kroute.c
> +++ b/usr.sbin/ripd/kroute.c
> @@ -183,8 +183,7 @@ kr_change_fib(struct kroute_node *kr, struct kroute
> *kroute, int action)
>
> if (kroute_insert(kr) == -1) {
> log_debug("kr_update_fib: cannot insert %s",
> - inet_ntoa(kr->r.nexthop));
> - free(kr);
> + inet_ntoa(kroute->nexthop));
> }
> } else
> kr->r.nexthop.s_addr = kroute->nexthop.s_addr;
> diff --git a/usr.sbin/ripd/ripe.c b/usr.sbin/ripd/ripe.c
> index d83901e245f..1f6f9b6583f 100644
> --- a/usr.sbin/ripd/ripe.c
> +++ b/usr.sbin/ripd/ripe.c
> @@ -351,6 +351,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
> NULL) {
> log_debug("unknown neighbor id %u",
> imsg.hdr.peerid);
> + free(rr);
> break;
> }
> add_entry(>rq_list, rr);
> @@ -396,6 +397,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
> if ((nbr = nbr_find_peerid(imsg.hdr.peerid)) == NULL) {
> log_debug("unknown neighbor id %u",
> imsg.hdr.peerid);
> + free(rr);
> break;
> }
> iface = nbr->iface;
>
> --
> Kind regards,
> Hiltjo
>
ripd: memory leak and double free/use-after-free
Hi,
While looking at the code of ripd:
I think there are (also) 2 small memleaks in a debug/error path
(IMSG_REQUEST_ADD and IMSG_RESPONSE_ADD). It breaks out before adding the
struct rip_route as an entry by the add_entry function (which adds it and adds
a reference count) in the log_debug block.
clang-analyzer also pointed at a double-free and use of free'd data: the
function kroute_insert frees kr and returns -1 when struct kroute is duplicate.
Patch below (untested):
diff --git a/usr.sbin/ripd/kroute.c b/usr.sbin/ripd/kroute.c
index 6e7449e0909..71758a75e44 100644
--- a/usr.sbin/ripd/kroute.c
+++ b/usr.sbin/ripd/kroute.c
@@ -183,8 +183,7 @@ kr_change_fib(struct kroute_node *kr, struct kroute
*kroute, int action)
if (kroute_insert(kr) == -1) {
log_debug("kr_update_fib: cannot insert %s",
- inet_ntoa(kr->r.nexthop));
- free(kr);
+ inet_ntoa(kroute->nexthop));
}
} else
kr->r.nexthop.s_addr = kroute->nexthop.s_addr;
diff --git a/usr.sbin/ripd/ripe.c b/usr.sbin/ripd/ripe.c
index d83901e245f..1f6f9b6583f 100644
--- a/usr.sbin/ripd/ripe.c
+++ b/usr.sbin/ripd/ripe.c
@@ -351,6 +351,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
NULL) {
log_debug("unknown neighbor id %u",
imsg.hdr.peerid);
+ free(rr);
break;
}
add_entry(>rq_list, rr);
@@ -396,6 +397,7 @@ ripe_dispatch_rde(int fd, short event, void *bula)
if ((nbr = nbr_find_peerid(imsg.hdr.peerid)) == NULL) {
log_debug("unknown neighbor id %u",
imsg.hdr.peerid);
+ free(rr);
break;
}
iface = nbr->iface;
--
Kind regards,
Hiltjo
