On 04/22/2010 10:37 AM, Atanu Ghosh wrote:
Hi,
The original code caught all exceptions from bad update packets in
peer.cc:BGPPeer::get_message. At the bottom of this routine you can
see the CorruptMessage exceptions being caught and the notification
being sent to the peer.
Your stack backtrace looks like the current code does not check the
update packet for sanity but just queues it, then later when the
packet is being processed it notices a problem and throws an
exception, which is not caught. Temporarily you could try catching the
exceptions around pull_next_route in the RibOutTable.
How does the attached patch look to you? It seems to fix
the problem for me...
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
diff --git a/bgp/path_attribute.cc b/bgp/path_attribute.cc
index b82475c..c02ba70 100644
--- a/bgp/path_attribute.cc
+++ b/bgp/path_attribute.cc
@@ -289,8 +289,10 @@ const
template <class A>
NextHopAttribute<A>::NextHopAttribute(const A& n)
+ throw(CorruptMessage)
: PathAttribute(Transitive, NEXT_HOP), _next_hop(n)
{
+ verify();
}
template <class A>
@@ -300,6 +302,22 @@ NextHopAttribute<A>::clone() const
return new NextHopAttribute(_next_hop);
}
+/* Throw exception if there are problems...do nothing
+ * otherwise.
+ */
+template <class A>
+void
+NextHopAttribute<A>::verify()
+ throw(CorruptMessage)
+{
+ //XLOG_ASSERT(!_next_hop.is_zero());
+ if (!_next_hop.is_unicast())
+ xorp_throw(CorruptMessage,
+ c_format("NextHop %s is not a unicast address",
+ _next_hop.str().c_str()),
+ UPDATEMSGERR, INVALNHATTR);
+}
+
template <class A>
NextHopAttribute<A>::NextHopAttribute(const uint8_t* d)
throw(CorruptMessage)
@@ -318,11 +336,7 @@ NextHopAttribute<A>::NextHopAttribute(const uint8_t* d)
_next_hop = A(payload(d));
- if (!(_next_hop.is_unicast() || _next_hop.is_zero()))
- xorp_throw(CorruptMessage,
- c_format("NextHop %s is not a unicast address",
- _next_hop.str().c_str()),
- UPDATEMSGERR, INVALNHATTR, d, total_tlv_length(d));
+ verify();
}
template<class A>
@@ -3410,13 +3424,18 @@ FastPathAttributeList<A>::str() const
if (_att[type]) {
s += "\n\t" + _att[type]->str();
} else if(_att_lengths[type]>0) {
- // we're got data for an attribute, but not decoded it yet
- size_t used = _att_lengths[type];
- PathAttribute *pa = PathAttribute::create(_att_bytes[type],
- _att_lengths[type],
- used, NULL,
A::ip_version());
- _att[type] = pa;
- s += "\n\t" + _att[type]->str();
+ try {
+ // we've got data for an attribute, but not decoded it yet
+ size_t used = _att_lengths[type];
+ PathAttribute *pa = PathAttribute::create(_att_bytes[type],
+ _att_lengths[type],
+ used, NULL,
A::ip_version());
+ _att[type] = pa;
+ s += "\n\t" + _att[type]->str();
+ }
+ catch (const XorpException& e) {
+ s += "\n\tException: " + e.str();
+ }
}
}
return s;
diff --git a/bgp/path_attribute.hh b/bgp/path_attribute.hh
index 7fc2b37..43959ef 100644
--- a/bgp/path_attribute.hh
+++ b/bgp/path_attribute.hh
@@ -349,10 +349,15 @@ template <class A>
class NextHopAttribute : public PathAttribute
{
public:
- NextHopAttribute(const A& n);
+ NextHopAttribute(const A& n) throw(CorruptMessage);
NextHopAttribute(const uint8_t* d) throw(CorruptMessage);
PathAttribute *clone() const;
+ /* Throw exception if there are problems...do nothing
+ * otherwise.
+ */
+ void verify() throw(CorruptMessage);
+
string str() const {
return "Next Hop Attribute " + _next_hop.str();
}
diff --git a/bgp/rib_ipc_handler.cc b/bgp/rib_ipc_handler.cc
index c034d20..e8ec0a6 100644
--- a/bgp/rib_ipc_handler.cc
+++ b/bgp/rib_ipc_handler.cc
@@ -279,11 +279,18 @@ RibIpcHandler::originate_route(const OriginType origin,
const ASPath& aspath,
origin, aspath.str().c_str(), nlri.str().c_str(),
next_hop.str().c_str(), unicast, multicast);
- /*
- ** Construct the path attribute list.
- */
- FPAList4Ref pa_list =
- new FastPathAttributeList<IPv4>(next_hop, aspath, origin);
+ FPAList4Ref pa_list;
+ try {
+ /*
+ ** Construct the path attribute list.
+ */
+ pa_list = new FastPathAttributeList<IPv4>(next_hop, aspath, origin);
+ }
+ catch (const XorpException& e) {
+ XLOG_WARNING("WARNING: Exception in originate_route: %s\n",
e.str().c_str());
+ // Returning false may cause more trouble than it's worth..
+ return true;
+ }
/*
** Add a local pref for I-BGP peers.
diff --git a/bgp/route_table_ribin.cc b/bgp/route_table_ribin.cc
index 74fbae7..fe58f9a 100644
--- a/bgp/route_table_ribin.cc
+++ b/bgp/route_table_ribin.cc
@@ -369,7 +369,7 @@ RibInTable<A>::dump_next_route(DumpIterator<A>& dump_iter)
if (chained_rt->is_winner() || dump_iter.peer_to_dump_to() == NULL) {
InternalMessage<A> rt_msg(chained_rt, _peer, _genid);
- log("dump route: " + rt_msg.net().str());
+ //XLOG_WARNING("dump route: %s", rt_msg.str().c_str());
int res = this->_next_table->route_dump(rt_msg,
(BGPRouteTable<A>*)this,
dump_iter.peer_to_dump_to());
if(res == ADD_FILTERED)
_______________________________________________
Xorp-hackers mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
