Fix concurrency issue in updating DBEntry flags

Fixes bug: #1375226

Cause:
DBEntry flag is updated from two mutually non-exclusive task. This results in corrupting the flags and unexpected free/delete on entry.
The problem is caused while setting "OnRemoveQ" flag on DBEntry as it can be done from any task context.

Fix:
Move OnRemoveQ out of DBEntry flags and make it as atomic bool variable.
Add assert to catch the case where route gets deleted with active paths
Add assert to catch the case where path is inserted to a deleted route
Added new test for repeated route update(from a agent with different nexthop) which discovered this bug.

Change-Id: I5df04f89f00959799a921e9db37388c0eb56334e

src/bgp/bgp_route.cc

@@ -24,6 +24,7 @@ BgpRoute::BgpRoute() {
 }
 
 BgpRoute::~BgpRoute() {
+    assert(GetPathList().empty());
 }
 
 //
@@ -38,6 +39,7 @@ const BgpPath *BgpRoute::BestPath() const {
 // Insert given path and redo path selection.
 //
 void BgpRoute::InsertPath(BgpPath *path) {
+    assert(!IsDeleted());
     const Path *prev_front = front();
 
     insert(path);

src/bgp/test/bgp_xmpp_inetvpn_test.cc

@@ -272,6 +272,49 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteChangeLocalPref) {
     agent_b_->SessionDown();
 }
 
+//
+// Agent flaps a route repeatedly.
+//
+TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteFlap) {
+    Configure();
+    task_util::WaitForIdle();
+
+    // Create XMPP Agent A connected to XMPP server X.
+    agent_a_.reset(
+        new test::NetworkAgentMock(&evm_, "agent-a", xs_x_->GetPort(),
+            "127.0.0.1", "127.0.0.1"));
+    TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished());
+
+    // Create XMPP Agent B connected to XMPP server Y.
+    agent_b_.reset(
+        new test::NetworkAgentMock(&evm_, "agent-b", xs_y_->GetPort(),
+            "127.0.0.2", "127.0.0.2"));
+    TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished());
+
+    // Register to blue instance
+    agent_a_->Subscribe("blue", 1);
+    agent_b_->Subscribe("blue", 1);
+
+    // Add route from agent A and change it repeatedly.
+    stringstream route_a;
+    route_a << "10.1.1.1/32";
+    for (int idx = 0; idx < 1024; ++idx) {
+        agent_a_->AddRoute("blue", route_a.str(), "192.168.1.1");
+        agent_a_->AddRoute("blue", route_a.str(), "192.168.1.2");
+    }
+
+    // Delete route from agent A.
+    agent_a_->DeleteRoute("blue", route_a.str());
+    task_util::WaitForIdle();
+
+    // Verify that route is deleted at agents A and B.
+    VerifyRouteNoExists(agent_a_, "blue", route_a.str());
+    VerifyRouteNoExists(agent_b_, "blue", route_a.str());
+
+    // Close the sessions.
+    agent_a_->SessionDown();
+    agent_b_->SessionDown();
+}
 
 class TestEnvironment : public ::testing::Environment {
     virtual ~TestEnvironment() { }

src/db/db_entry.h

@@ -7,6 +7,8 @@
 
 #include <map>
 
+#include <tbb/atomic.h>
+
 #include "db/db_table.h"
 
 #include <boost/intrusive/list.hpp>
@@ -23,6 +25,7 @@ class DBEntryBase {
     typedef std::auto_ptr<DBRequestKey> KeyPtr;
 
     DBEntryBase() : tpart_(NULL), flags(0), last_change_at_(UTCTimestampUsec()) {
+        onremoveq_ = false;
     }
     virtual ~DBEntryBase() { }
     virtual std::string ToString() const = 0;
@@ -46,9 +49,13 @@ class DBEntryBase {
     void clear_onlist() { flags &= ~Onlist; }
     bool is_onlist() { return (flags & Onlist); }
 
-    void SetOnRemoveQ() { flags |= OnRemoveQ; }
-    void ClearOnRemoveQ() { flags &= ~OnRemoveQ; }
-    bool IsOnRemoveQ() { return (flags & OnRemoveQ); }
+    void SetOnRemoveQ() {
+        onremoveq_.fetch_and_store(true);
+    }
+    bool IsOnRemoveQ() { return (onremoveq_); }
+    void ClearOnRemoveQ() {
+        onremoveq_.fetch_and_store(false);
+    }
 
     //member hook in change list
     boost::intrusive::list_member_hook<> chg_list_;
@@ -65,12 +72,12 @@ class DBEntryBase {
     enum DbEntryFlags {
         Onlist       = 1 << 0,
         DeleteMarked = 1 << 1,
-        OnRemoveQ    = 1 << 2,
     };
     typedef std::map<ListenerId, DBState *> StateMap;
     DBTablePartBase *tpart_;
     StateMap state_;
     uint8_t flags;
+    tbb::atomic<bool> onremoveq_;
     uint64_t last_change_at_; // time at which entry was last 'changed'
     DISALLOW_COPY_AND_ASSIGN(DBEntryBase);
 };