Commit cfbdd869 for quagga.net
commit cfbdd869687dc076256bce455d03b2ccf04b4a77
Author: Paul Jakma <paul.jakma@hpe.com>
Date: Mon Apr 3 22:02:38 2017 +0100
lib/filter: change add/delete callback hooks to robustly delete
* Prior band-aids were made to address use-after-frees in lib/filter
with deletes, but they introduced another error. They allowed the
access-list being deleted to be visible by access_list_lookup from
the users' delete callback, causing deleted access-list references to
not be removed, leading to different use-after-frees instead.
Fix in a robust manner within the filter code.
This bug was reported and debugged by matt@kontoff.com, with an
initial fix, on which this commit builds. See
https://bugzilla.quagga.net/show_bug.cgi?id=945.
* filter.h: Change the callback hooks to take the access-list name, not
the access-list reference. The name can be a weaker, more opaque
ref.
* filter.c: Update hooks calls to pass name.
(access_list_{insert,lookup})
guard strcmp of name, as name may now potentially be NULL for access-lists
in process of being deleted.
(access_list_filter_delete) Transfer ownership of the access-list name
to a local, so the access-list can be deleted, and the name then passed
to the callback.
(no_access_list_all) ditto.
(no_ipv6_access_list_all) ditto.
(filter_show) guard name strcmp, shouldn't be possible to see an access-list
being deleted here, but better safe.
* ospfd/ospf_zebra.c: (ospf_filter_update) adjust for hook change.
* bgpd/bgpd.c: (peer_distribute_update) adjust for hook change, not using the
arg.
* ripd/ripd.c: (rip_distribute_update_all_wrapper) ditto
* ripngd/ripngd.c: (ripng_distribute_update_all_wrapper) ditto.
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index 6aeecb13..5869d35f 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -3938,7 +3938,7 @@ peer_distribute_unset (struct peer *peer, afi_t afi, safi_t safi, int direct)
/* Update distribute list. */
static void
-peer_distribute_update (struct access_list *access)
+peer_distribute_update (const char *name)
{
afi_t afi;
safi_t safi;
diff --git a/lib/filter.c b/lib/filter.c
index ffecd30d..ffb943f6 100644
--- a/lib/filter.c
+++ b/lib/filter.c
@@ -85,10 +85,10 @@ struct access_master
struct access_list_list str;
/* Hook function which is executed when new access_list is added. */
- void (*add_hook) (struct access_list *);
+ void (*add_hook) (const char *);
/* Hook function which is executed when access_list is deleted. */
- void (*delete_hook) (struct access_list *);
+ void (*delete_hook) (const char *);
};
/* Static structure for IPv4 access_list's master. */
@@ -317,7 +317,7 @@ access_list_insert (afi_t afi, const char *name)
/* Set point to insertion point. */
for (point = alist->head; point; point = point->next)
- if (strcmp (point->name, name) >= 0)
+ if (point->name && strcmp (point->name, name) >= 0)
break;
}
@@ -372,11 +372,11 @@ access_list_lookup (afi_t afi, const char *name)
return NULL;
for (access = master->num.head; access; access = access->next)
- if (strcmp (access->name, name) == 0)
+ if (access->name && strcmp (access->name, name) == 0)
return access;
for (access = master->str.head; access; access = access->next)
- if (strcmp (access->name, name) == 0)
+ if (access->name && strcmp (access->name, name) == 0)
return access;
return NULL;
@@ -426,7 +426,7 @@ access_list_apply (struct access_list *access, void *object)
/* Add hook function. */
void
-access_list_add_hook (void (*func) (struct access_list *access))
+access_list_add_hook (void (*func) (const char *))
{
access_master_ipv4.add_hook = func;
#ifdef HAVE_IPV6
@@ -436,7 +436,7 @@ access_list_add_hook (void (*func) (struct access_list *access))
/* Delete hook function. */
void
-access_list_delete_hook (void (*func) (struct access_list *access))
+access_list_delete_hook (void (*func) (const char *))
{
access_master_ipv4.delete_hook = func;
#ifdef HAVE_IPV6
@@ -459,7 +459,7 @@ access_list_filter_add (struct access_list *access, struct filter *filter)
/* Run hook function. */
if (access->master->add_hook)
- (*access->master->add_hook) (access);
+ (*access->master->add_hook) (access->name);
}
/* If access_list has no filter then return 1. */
@@ -478,7 +478,22 @@ static void
access_list_filter_delete (struct access_list *access, struct filter *filter)
{
struct access_master *master;
-
+ /* transfer ownership of access->name to a local, to retain the name
+ * to pass to a delete hook, while the access-list is deleted
+ *
+ * It is important that access-lists that are deleted, or are in process
+ * of being deleted, are not visible via access_list_lookup. This is
+ * because some (all?) users process the delete_hook callback the same
+ * as an add - they simply refresh all their access_list name references
+ * by looking up the name.
+ *
+ * If an access list can be looked up while being deleted, such users will
+ * not remove an access-list, and will keep dangling references to
+ * freed access lists.
+ */
+ char *name = access->name;
+ access->name = NULL;
+
master = access->master;
if (filter->next)
@@ -492,14 +507,16 @@ access_list_filter_delete (struct access_list *access, struct filter *filter)
access->head = filter->next;
filter_free (filter);
-
+
/* If access_list becomes empty delete it from access_master. */
if (access_list_empty (access))
access_list_delete (access);
-
+
/* Run hook function. */
if (master->delete_hook)
- (*master->delete_hook) (access);
+ (*master->delete_hook) (name);
+
+ XFREE (MTYPE_ACCESS_LIST_STR, name);
}
/*
@@ -1325,7 +1342,8 @@ DEFUN (no_access_list_all,
{
struct access_list *access;
struct access_master *master;
-
+ char *name;
+
/* Looking up access_list. */
access = access_list_lookup (AFI_IP, argv[0]);
if (access == NULL)
@@ -1336,14 +1354,20 @@ DEFUN (no_access_list_all,
}
master = access->master;
-
+ /* transfer ownership of access->name to a local, to retain
+ * a while longer, past access_list being freed */
+ name = access->name;
+ access->name = NULL;
+
/* Delete all filter from access-list. */
access_list_delete (access);
/* Run hook function. */
if (master->delete_hook)
- (*master->delete_hook) (access);
+ (*master->delete_hook) (name);
+ XFREE (MTYPE_ACCESS_LIST_STR, name);
+
return CMD_SUCCESS;
}
@@ -1496,7 +1520,8 @@ DEFUN (no_ipv6_access_list_all,
{
struct access_list *access;
struct access_master *master;
-
+ char *name;
+
/* Looking up access_list. */
access = access_list_lookup (AFI_IP6, argv[0]);
if (access == NULL)
@@ -1507,14 +1532,17 @@ DEFUN (no_ipv6_access_list_all,
}
master = access->master;
-
+ name = access->name;
+ access->name = NULL;
+
/* Delete all filter from access-list. */
access_list_delete (access);
/* Run hook function. */
if (master->delete_hook)
- (*master->delete_hook) (access);
+ (*master->delete_hook) (name);
+ XFREE (MTYPE_ACCESS_LIST_STR, name);
return CMD_SUCCESS;
}
@@ -1588,7 +1616,7 @@ filter_show (struct vty *vty, const char *name, afi_t afi)
for (access = master->num.head; access; access = access->next)
{
- if (name && strcmp (access->name, name) != 0)
+ if (!access->name || (name && strcmp (access->name, name) != 0))
continue;
write = 1;
@@ -1631,7 +1659,7 @@ filter_show (struct vty *vty, const char *name, afi_t afi)
for (access = master->str.head; access; access = access->next)
{
- if (name && strcmp (access->name, name) != 0)
+ if (!access->name || (name && strcmp (access->name, name) != 0))
continue;
write = 1;
diff --git a/lib/filter.h b/lib/filter.h
index e6ccd33b..85d01587 100644
--- a/lib/filter.h
+++ b/lib/filter.h
@@ -64,8 +64,8 @@ struct access_list
/* Prototypes for access-list. */
extern void access_list_init (void);
extern void access_list_reset (void);
-extern void access_list_add_hook (void (*func)(struct access_list *));
-extern void access_list_delete_hook (void (*func)(struct access_list *));
+extern void access_list_add_hook (void (*func) (const char *));
+extern void access_list_delete_hook (void (*func) (const char *));
extern struct access_list *access_list_lookup (afi_t, const char *);
extern enum filter_type access_list_apply (struct access_list *, void *);
diff --git a/ospfd/ospf_zebra.c b/ospfd/ospf_zebra.c
index 60d98529..95d2afed 100644
--- a/ospfd/ospf_zebra.c
+++ b/ospfd/ospf_zebra.c
@@ -1069,7 +1069,7 @@ ospf_distribute_list_update (struct ospf *ospf, uintptr_t type)
/* If access-list is updated, apply some check. */
static void
-ospf_filter_update (struct access_list *access)
+ospf_filter_update (const char *name)
{
struct ospf *ospf;
int type;
@@ -1112,7 +1112,7 @@ ospf_filter_update (struct access_list *access)
/* Schedule distribute-list update timer. */
if (DISTRIBUTE_LIST (ospf, type) == NULL ||
- strcmp (DISTRIBUTE_NAME (ospf, type), access->name) == 0)
+ strcmp (DISTRIBUTE_NAME (ospf, type), name) == 0)
ospf_distribute_list_update (ospf, type);
}
}
diff --git a/ripd/ripd.c b/ripd/ripd.c
index 3866624d..8f7e077f 100644
--- a/ripd/ripd.c
+++ b/ripd/ripd.c
@@ -3872,7 +3872,7 @@ rip_distribute_update_all (struct prefix_list *notused)
}
/* ARGSUSED */
static void
-rip_distribute_update_all_wrapper(struct access_list *notused)
+rip_distribute_update_all_wrapper(const char *unused)
{
rip_distribute_update_all(NULL);
}
diff --git a/ripngd/ripngd.c b/ripngd/ripngd.c
index 824b3a4c..1510b1df 100644
--- a/ripngd/ripngd.c
+++ b/ripngd/ripngd.c
@@ -2827,7 +2827,7 @@ ripng_distribute_update_all (struct prefix_list *notused)
}
static void
-ripng_distribute_update_all_wrapper (struct access_list *notused)
+ripng_distribute_update_all_wrapper (const char *notused)
{
ripng_distribute_update_all(NULL);
}