[ospfd] Allow ospf_lsa_unlock to NULL out callers' LSA pointers upon free
2006-07-26 Paul Jakma <paul.jakma@sun.com>
* ospf_lsa.{c,h}: (ospf_lsa_unlock) Change to take a double pointer
to the LSA to be 'unlocked', so that, if the LSA is freed, the
callers pointer to the LSA can be NULLed out, allowing any further
use of that pointer to provoke a crash sooner rather than later.
* ospf_*.c: (general) Adjust callers of ospf_lsa_unlock to match
previous. Try annotate 'locking' somewhat to show which 'locks'
are protecting what LSA reference, if not obvious.
* ospf_opaque.c: (ospf_opaque_lsa_install) Trivial: remove useless
goto, replace with return.
* ospf_packet.c: (ospf_make_ls_ack) Trivial: merge two list loops,
the dual-loop predated the delete-safe list-loop macro.
diff --git a/ospfd/ChangeLog b/ospfd/ChangeLog
index 2c23774..61ca5e0 100644
--- a/ospfd/ChangeLog
+++ b/ospfd/ChangeLog
@@ -1,3 +1,17 @@
+2006-07-26 Paul Jakma <paul.jakma@sun.com>
+
+ * ospf_lsa.{c,h}: (ospf_lsa_unlock) Change to take a double pointer
+ to the LSA to be 'unlocked', so that, if the LSA is freed, the
+ callers pointer to the LSA can be NULLed out, allowing any further
+ use of that pointer to provoke a crash sooner rather than later.
+ * ospf_*.c: (general) Adjust callers of ospf_lsa_unlock to match
+ previous. Try annotate 'locking' somewhat to show which 'locks'
+ are protecting what LSA reference, if not obvious.
+ * ospf_opaque.c: (ospf_opaque_lsa_install) Trivial: remove useless
+ goto, replace with return.
+ * ospf_packet.c: (ospf_make_ls_ack) Trivial: merge two list loops,
+ the dual-loop predated the delete-safe list-loop macro.
+
2006-07-25 Paul Jakma <paul.jakma@sun.com>
* ospf_neigbor.h: (struct ospf_neighbor) Add some additional
diff --git a/ospfd/ospf_apiserver.c b/ospfd/ospf_apiserver.c
index 2ee4d3e..dac4c93 100644
--- a/ospfd/ospf_apiserver.c
+++ b/ospfd/ospf_apiserver.c
@@ -1520,7 +1520,7 @@
if ((new->data = ospf_lsa_data_new (length)) == NULL)
{
zlog_warn ("ospf_apiserver_opaque_lsa_new: ospf_lsa_data_new() ?");
- ospf_lsa_unlock (new);
+ ospf_lsa_unlock (&new);
stream_free (s);
return NULL;
}
@@ -1885,7 +1885,7 @@
if (ospf_lsa_install (ospf, new->oi, new) == NULL)
{
zlog_warn ("ospf_apiserver_lsa_refresher: ospf_lsa_install failed");
- ospf_lsa_unlock (new);
+ ospf_lsa_unlock (&new);
goto out;
}
diff --git a/ospfd/ospf_ase.c b/ospfd/ospf_ase.c
index f4b285b..a481234 100644
--- a/ospfd/ospf_ase.c
+++ b/ospfd/ospf_ase.c
@@ -711,7 +711,7 @@
/* We assume that if LSA is deleted from DB
is is also deleted from this RT */
- listnode_add (lst, ospf_lsa_lock (lsa));
+ listnode_add (lst, ospf_lsa_lock (lsa)); /* external_lsas lst */
}
void
@@ -730,18 +730,12 @@
rn = route_node_get (top->external_lsas, (struct prefix *) &p);
lst = rn->info;
-#ifdef ORIGINAL_CODING
- assert (lst);
- listnode_delete (lst, lsa);
- ospf_lsa_unlock (lsa);
-#else /* ORIGINAL_CODING */
/* XXX lst can be NULL */
if (lst) {
listnode_delete (lst, lsa);
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* external_lsas list */
}
-#endif /* ORIGINAL_CODING */
}
void
@@ -756,7 +750,7 @@
if ((lst = rn->info) != NULL)
{
for (ALL_LIST_ELEMENTS (lst, node, nnode, lsa))
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* external_lsas lst */
list_delete (lst);
}
diff --git a/ospfd/ospf_flood.c b/ospfd/ospf_flood.c
index d7ab859..91cbbf3 100644
--- a/ospfd/ospf_flood.c
+++ b/ospfd/ospf_flood.c
@@ -72,7 +72,7 @@
return;
/* Schedule a delayed LSA Ack to be sent */
- listnode_add (inbr->oi->ls_ack, ospf_lsa_lock (lsa));
+ listnode_add (inbr->oi->ls_ack, ospf_lsa_lock (lsa)); /* delayed LSA Ack */
}
/* Check LSA is related to external info. */
@@ -134,7 +134,7 @@
case OSPF_ROUTER_LSA:
/* Originate a new instance and schedule flooding */
/* It shouldn't be necessary, but anyway */
- ospf_lsa_unlock (area->router_lsa_self);
+ ospf_lsa_unlock (&area->router_lsa_self);
area->router_lsa_self = ospf_lsa_lock (new);
ospf_router_lsa_timer_add (area);
@@ -170,7 +170,7 @@
}
#endif /* HAVE_OPAQUE_LSA */
- ospf_lsa_unlock (oi->network_lsa_self);
+ ospf_lsa_unlock (&oi->network_lsa_self);
oi->network_lsa_self = ospf_lsa_lock (new);
/* Schedule network-LSA origination. */
@@ -797,7 +797,7 @@
{
if (nbr->ls_req_last == lsa)
{
- ospf_lsa_unlock (nbr->ls_req_last);
+ ospf_lsa_unlock (&nbr->ls_req_last);
nbr->ls_req_last = NULL;
}
@@ -813,7 +813,7 @@
void
ospf_ls_request_delete_all (struct ospf_neighbor *nbr)
{
- ospf_lsa_unlock (nbr->ls_req_last);
+ ospf_lsa_unlock (&nbr->ls_req_last);
nbr->ls_req_last = NULL;
ospf_lsdb_delete_all (&nbr->ls_req);
}
@@ -922,7 +922,7 @@
ospf_ls_retransmit_delete (nbr, lsa);
}
- ospf_lsa_unlock (nbr->ls_req_last);
+ ospf_lsa_unlock (&nbr->ls_req_last);
nbr->ls_req_last = NULL;
}
diff --git a/ospfd/ospf_interface.c b/ospfd/ospf_interface.c
index 2c2c074..31275f8 100644
--- a/ospfd/ospf_interface.c
+++ b/ospfd/ospf_interface.c
@@ -289,7 +289,7 @@
/* Cleanup Link State Acknowlegdment list. */
for (ALL_LIST_ELEMENTS (oi->ls_ack, node, nnode, lsa))
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* oi->ls_ack */
list_delete_all_node (oi->ls_ack);
oi->crypt_seqnum = 0;
@@ -302,7 +302,7 @@
oi->nbr_self = ospf_nbr_new (oi);
ospf_nbr_add_self (oi);
- ospf_lsa_unlock (oi->network_lsa_self);
+ ospf_lsa_unlock (&oi->network_lsa_self);
oi->network_lsa_self = NULL;
OSPF_TIMER_OFF (oi->t_network_lsa_self);
}
diff --git a/ospfd/ospf_ism.c b/ospfd/ospf_ism.c
index 0875e92..829ea00 100644
--- a/ospfd/ospf_ism.c
+++ b/ospfd/ospf_ism.c
@@ -593,7 +593,7 @@
OSPF_TIMER_OFF (oi->t_network_lsa_self);
}
- ospf_lsa_unlock (oi->network_lsa_self);
+ ospf_lsa_unlock (&oi->network_lsa_self);
oi->network_lsa_self = NULL;
}
diff --git a/ospfd/ospf_lsa.c b/ospfd/ospf_lsa.c
index 8b5c6eb..b99b931 100644
--- a/ospfd/ospf_lsa.c
+++ b/ospfd/ospf_lsa.c
@@ -294,20 +294,21 @@
/* Unlock LSA. */
void
-ospf_lsa_unlock (struct ospf_lsa *lsa)
+ospf_lsa_unlock (struct ospf_lsa **lsa)
{
/* This is sanity check. */
- if (!lsa)
+ if (!lsa || !*lsa)
return;
- lsa->lock--;
+ (*lsa)->lock--;
- assert (lsa->lock >= 0);
+ assert ((*lsa)->lock >= 0);
- if (lsa->lock == 0)
+ if ((*lsa)->lock == 0)
{
- assert (CHECK_FLAG (lsa->flags, OSPF_LSA_DISCARD));
- ospf_lsa_free (lsa);
+ assert (CHECK_FLAG ((*lsa)->flags, OSPF_LSA_DISCARD));
+ ospf_lsa_free (*lsa);
+ *lsa = NULL;
}
}
@@ -318,7 +319,7 @@
if (!CHECK_FLAG (lsa->flags, OSPF_LSA_DISCARD))
{
SET_FLAG (lsa->flags, OSPF_LSA_DISCARD);
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa);
}
}
@@ -1044,7 +1045,7 @@
zlog_debug("LSA[Type%d:%s]: Refresh router-LSA for Area %s",
lsa->data->type, inet_ntoa (lsa->data->id), area_str);
ospf_lsa_flush_area (lsa, area);
- ospf_lsa_unlock (area->router_lsa_self);
+ ospf_lsa_unlock (&area->router_lsa_self);
area->router_lsa_self = NULL;
/* Refresh router-LSA, (not install) and flood through area. */
@@ -1850,7 +1851,7 @@
{
if (IS_DEBUG_OSPF_NSSA)
zlog_debug ("LSA[Type-7]: Could not build FWD-ADDR");
- ospf_lsa_discard(new);
+ ospf_lsa_discard (new);
return;
}
}
@@ -2518,7 +2519,7 @@
ospf_router_lsa_timer, OSPF_LS_REFRESH_TIME);
/* Set self-originated router-LSA. */
- ospf_lsa_unlock (area->router_lsa_self);
+ ospf_lsa_unlock (&area->router_lsa_self);
area->router_lsa_self = ospf_lsa_lock (new);
if (IS_DEBUG_OSPF (lsa, LSA_INSTALL))
@@ -2562,7 +2563,7 @@
ospf_network_lsa_refresh_timer,
OSPF_LS_REFRESH_TIME);
- ospf_lsa_unlock (oi->network_lsa_self);
+ ospf_lsa_unlock (&oi->network_lsa_self);
oi->network_lsa_self = ospf_lsa_lock (new);
}
@@ -3067,7 +3068,7 @@
if ((n = listnode_lookup (ospf->maxage_lsa, lsa)))
{
list_delete_node (ospf->maxage_lsa, n);
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* maxage_lsa */
}
}
@@ -3482,7 +3483,7 @@
zlog_debug ("LSA[Type%d:%s]: Schedule self-originated LSA to FLUSH", lsa->data->type, inet_ntoa (lsa->data->id));
ospf_lsa_flush_area (lsa, area);
- ospf_lsa_unlock (area->router_lsa_self);
+ ospf_lsa_unlock (&area->router_lsa_self);
area->router_lsa_self = NULL;
OSPF_TIMER_OFF (area->t_router_lsa_self);
}
@@ -3497,7 +3498,7 @@
zlog_debug ("LSA[Type%d:%s]: Schedule self-originated LSA to FLUSH", lsa->data->type, inet_ntoa (lsa->data->id));
ospf_lsa_flush_area (oi->network_lsa_self, area);
- ospf_lsa_unlock (oi->network_lsa_self);
+ ospf_lsa_unlock (&oi->network_lsa_self);
oi->network_lsa_self = NULL;
OSPF_TIMER_OFF (oi->t_network_lsa_self);
}
@@ -3666,7 +3667,7 @@
break;
}
- ospf_lsa_unlock (data->lsa);
+ ospf_lsa_unlock (&data->lsa); /* Message */
XFREE (MTYPE_OSPF_MESSAGE, data);
return 0;
}
@@ -3681,7 +3682,7 @@
data->action = LSA_ACTION_FLOOD_AREA;
data->area = area;
- data->lsa = ospf_lsa_lock (lsa);
+ data->lsa = ospf_lsa_lock (lsa); /* Message / Flood area */
thread_add_event (master, ospf_lsa_action, data, 0);
}
@@ -3696,7 +3697,7 @@
data->action = LSA_ACTION_FLUSH_AREA;
data->area = area;
- data->lsa = ospf_lsa_lock (lsa);
+ data->lsa = ospf_lsa_lock (lsa); /* Message / Flush area */
thread_add_event (master, ospf_lsa_action, data, 0);
}
@@ -3779,7 +3780,8 @@
inet_ntoa (lsa->data->id), LS_AGE (lsa), index);
if (!ospf->lsa_refresh_queue.qs[index])
ospf->lsa_refresh_queue.qs[index] = list_new ();
- listnode_add (ospf->lsa_refresh_queue.qs[index], ospf_lsa_lock (lsa));
+ listnode_add (ospf->lsa_refresh_queue.qs[index],
+ ospf_lsa_lock (lsa)); /* lsa_refresh_queue */
lsa->refresh_list = index;
if (IS_DEBUG_OSPF (lsa, LSA_REFRESH))
zlog_debug ("LSA[Refresh:%s]: ospf_refresher_register_lsa(): "
@@ -3801,7 +3803,7 @@
list_free (refresh_list);
ospf->lsa_refresh_queue.qs[lsa->refresh_list] = NULL;
}
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* lsa_refresh_queue */
lsa->refresh_list = -1;
}
}
@@ -3855,7 +3857,7 @@
inet_ntoa (lsa->data->id), lsa, i);
list_delete_node (refresh_list, node);
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* lsa_refresh_queue */
lsa->refresh_list = -1;
listnode_add (lsa_to_refresh, lsa);
}
diff --git a/ospfd/ospf_lsa.h b/ospfd/ospf_lsa.h
index 9e480de..8dd054c 100644
--- a/ospfd/ospf_lsa.h
+++ b/ospfd/ospf_lsa.h
@@ -245,7 +245,7 @@
extern struct ospf_lsa *ospf_lsa_dup (struct ospf_lsa *);
extern void ospf_lsa_free (struct ospf_lsa *);
extern struct ospf_lsa *ospf_lsa_lock (struct ospf_lsa *);
-extern void ospf_lsa_unlock (struct ospf_lsa *);
+extern void ospf_lsa_unlock (struct ospf_lsa **);
extern void ospf_lsa_discard (struct ospf_lsa *);
extern struct lsa_header *ospf_lsa_data_new (size_t);
diff --git a/ospfd/ospf_lsdb.c b/ospfd/ospf_lsdb.c
index b161b80..28d92bd 100644
--- a/ospfd/ospf_lsdb.c
+++ b/ospfd/ospf_lsdb.c
@@ -108,7 +108,7 @@
old = rn->info;
lsdb->type[old->data->type].checksum -= ntohs(old->data->checksum);
- ospf_lsa_unlock (rn->info);
+ ospf_lsa_unlock (&rn->info);
route_unlock_node (rn);
}
@@ -161,7 +161,7 @@
if (lsdb->del_lsa_hook != NULL)
(* lsdb->del_lsa_hook)(lsa);
#endif /* MONITOR_LSDB_CHANGE */
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa);
return;
}
}
@@ -191,7 +191,7 @@
if (lsdb->del_lsa_hook != NULL)
(* lsdb->del_lsa_hook)(lsa);
#endif /* MONITOR_LSDB_CHANGE */
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa);
}
}
}
diff --git a/ospfd/ospf_nsm.c b/ospfd/ospf_nsm.c
index 8329a4f..e3517cd 100644
--- a/ospfd/ospf_nsm.c
+++ b/ospfd/ospf_nsm.c
@@ -723,7 +723,7 @@
if (oi->network_lsa_self && oi->full_nbrs == 0)
{
ospf_lsa_flush_area (oi->network_lsa_self, oi->area);
- ospf_lsa_unlock (oi->network_lsa_self);
+ ospf_lsa_unlock (&oi->network_lsa_self);
oi->network_lsa_self = NULL;
OSPF_TIMER_OFF (oi->t_network_lsa_self);
}
diff --git a/ospfd/ospf_opaque.c b/ospfd/ospf_opaque.c
index f2496cf..0b6ac4c 100644
--- a/ospfd/ospf_opaque.c
+++ b/ospfd/ospf_opaque.c
@@ -708,7 +708,7 @@
OSPF_TIMER_OFF (oipi->t_opaque_lsa_self);
if (oipi->lsa != NULL)
- ospf_lsa_unlock (oipi->lsa);
+ ospf_lsa_unlock (&oipi->lsa);
XFREE (MTYPE_OPAQUE_INFO_PER_ID, oipi);
return;
}
@@ -1554,7 +1554,7 @@
if ((oipt = lookup_opaque_info_by_type (lsa)) != NULL
&& (oipi = lookup_opaque_info_by_id (oipt, lsa)) != NULL)
{
- ospf_lsa_unlock (oipi->lsa);
+ ospf_lsa_unlock (&oipi->lsa);
oipi->lsa = ospf_lsa_lock (lsa);
}
/* Register the new lsa entry and get its control info. */
@@ -2234,7 +2234,7 @@
u_char before;
if ((top = oi_to_top (nbr->oi)) == NULL)
- goto out;
+ return;
before = IS_OPAQUE_LSA_ORIGINATION_BLOCKED (top->opaque);
@@ -2259,7 +2259,7 @@
break;
default:
zlog_warn ("ospf_opaque_self_originated_lsa_received: Unexpected LSA-type(%u)", lsa->data->type);
- goto out;
+ return;
}
ospf_lsa_discard (lsa); /* List "lsas" will be deleted by caller. */
@@ -2269,9 +2269,6 @@
if (IS_DEBUG_OSPF_EVENT)
zlog_debug ("Block Opaque-LSA origination: OFF -> ON");
}
-
-out:
- return;
}
void
diff --git a/ospfd/ospf_packet.c b/ospfd/ospf_packet.c
index 788daba..44dca18 100644
--- a/ospfd/ospf_packet.c
+++ b/ospfd/ospf_packet.c
@@ -2764,7 +2764,7 @@
stream_put_ipv4 (s, lsa->data->id.s_addr);
stream_put_ipv4 (s, lsa->data->adv_router.s_addr);
- ospf_lsa_unlock (nbr->ls_req_last);
+ ospf_lsa_unlock (&nbr->ls_req_last);
nbr->ls_req_last = ospf_lsa_lock (lsa);
*length += 12;
@@ -2860,7 +2860,7 @@
count++;
list_delete_node (update, node);
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* oi->ls_upd_queue */
}
/* Now set #LSAs. */
@@ -2874,17 +2874,13 @@
static int
ospf_make_ls_ack (struct ospf_interface *oi, struct list *ack, struct stream *s)
{
- struct list *rm_list;
- struct listnode *node;
+ struct listnode *node, *nnode;
u_int16_t length = OSPF_LS_ACK_MIN_SIZE;
unsigned long delta = stream_get_endp(s) + 24;
struct ospf_lsa *lsa;
- rm_list = list_new ();
-
- for (ALL_LIST_ELEMENTS_RO (ack, node, lsa))
+ for (ALL_LIST_ELEMENTS (ack, node, nnode, lsa))
{
- lsa = listgetdata (node);
assert (lsa);
if (length + delta > ospf_packet_max (oi))
@@ -2893,21 +2889,10 @@
stream_put (s, lsa->data, OSPF_LSA_HEADER_SIZE);
length += OSPF_LSA_HEADER_SIZE;
- listnode_add (rm_list, lsa);
- }
-
- /* Remove LSA from LS-Ack list. */
- /* XXX: this loop should be removed and the list move done in previous
- * loop
- */
- for (ALL_LIST_ELEMENTS_RO (rm_list, node, lsa))
- {
listnode_delete (ack, lsa);
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* oi->ls_ack_direct.ls_ack */
}
- list_delete (rm_list);
-
return length;
}
@@ -3396,10 +3381,7 @@
rn->info = list_new ();
for (ALL_LIST_ELEMENTS_RO (update, node, lsa))
- {
- ospf_lsa_lock (lsa);
- listnode_add (rn->info, lsa);
- }
+ listnode_add (rn->info, ospf_lsa_lock (lsa)); /* oi->ls_upd_queue */
if (oi->t_ls_upd_event == NULL)
oi->t_ls_upd_event =
diff --git a/ospfd/ospf_te.c b/ospfd/ospf_te.c
index 10a94b8..a3ebe62 100644
--- a/ospfd/ospf_te.c
+++ b/ospfd/ospf_te.c
@@ -904,7 +904,7 @@
if ((new->data = ospf_lsa_data_new (length)) == NULL)
{
zlog_warn ("ospf_mpls_te_lsa_new: ospf_lsa_data_new() ?");
- ospf_lsa_unlock (new);
+ ospf_lsa_unlock (&new);
new = NULL;
stream_free (s);
goto out;
@@ -936,7 +936,7 @@
if (ospf_lsa_install (area->ospf, NULL/*oi*/, new) == NULL)
{
zlog_warn ("ospf_mpls_te_lsa_originate1: ospf_lsa_install() ?");
- ospf_lsa_unlock (new);
+ ospf_lsa_unlock (&new);
goto out;
}
@@ -1054,7 +1054,7 @@
if (ospf_lsa_install (area->ospf, NULL/*oi*/, new) == NULL)
{
zlog_warn ("ospf_mpls_te_lsa_refresh: ospf_lsa_install() ?");
- ospf_lsa_unlock (new);
+ ospf_lsa_unlock (&new);
goto out;
}
diff --git a/ospfd/ospfd.c b/ospfd/ospfd.c
index 79c4543..ef8272b 100644
--- a/ospfd/ospfd.c
+++ b/ospfd/ospfd.c
@@ -475,7 +475,7 @@
ospf_lsdb_free (ospf->lsdb);
for (ALL_LIST_ELEMENTS (ospf->maxage_lsa, node, nnode, lsa))
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* maxage_lsa */
list_delete (ospf->maxage_lsa);
@@ -592,7 +592,7 @@
ospf_lsdb_delete_all (area->lsdb);
ospf_lsdb_free (area->lsdb);
- ospf_lsa_unlock (area->router_lsa_self);
+ ospf_lsa_unlock (&area->router_lsa_self);
route_table_finish (area->ranges);
list_delete (area->oiflist);
@@ -905,7 +905,7 @@
if ((lst = (struct list *) rn->info))
{
for (ALL_LIST_ELEMENTS (lst, node, nnode, lsa))
- ospf_lsa_unlock (lsa);
+ ospf_lsa_unlock (&lsa); /* oi->ls_upd_queue */
list_free (lst);
rn->info = NULL;
}