From: Tomek Mrugalski <tomasz@isc.org>
Date: Fri, 21 Apr 2017 15:42:21 +0000 (+0200)
Subject: [5208a] hook points can be registered several times
X-Git-Tag: trac5212_base~10^2~9
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=35f04448ae73464dfb4e5e7f46730316eb995554;p=thirdparty%2Fkea.git

[5208a] hook points can be registered several times

 - see ticket #5251 for explanation of the problem
---

diff --git a/src/lib/hooks/server_hooks.cc b/src/lib/hooks/server_hooks.cc
index 504446cb40..412b7e2c1f 100644
--- a/src/lib/hooks/server_hooks.cc
+++ b/src/lib/hooks/server_hooks.cc
@@ -46,10 +46,24 @@ ServerHooks::registerHook(const string& name) {
         hooks_.insert(make_pair(name, index));
 
     if (!result.second) {
+
+        // There's a problem with hook libraries that need to be linked with
+        // libdhcpsrv. For example host_cmds hook library requires host
+        // parser, so it needs to be linked with libdhcpsrv. However, when
+        // unit-tests are started, the hook points are not registered.
+        // When the library is loaded new hook points are registered.
+        // This causes issues in the hooks framework, especially when
+        // LibraryManager::unloadLibrary() iterates through all hooks
+        // and then calls deregisterAllCallouts. This method gets
+        // hook_index that is greater than number of elements in
+        // hook_vector_ and then we have a read past the array boundary.
+        /// @todo: See ticket 5251 and 5208 for details.
+        return (getIndex(name));
+
         // New element was not inserted because an element with the same name
         // already existed.
-        isc_throw(DuplicateHook, "hook with name " << name <<
-                  " is already registered");
+        //isc_throw(DuplicateHook, "hook with name " << name <<
+        //         " is already registered");
     }
 
     // Element was inserted, so add to the inverse hooks collection.
diff --git a/src/lib/hooks/tests/hooks_manager_unittest.cc b/src/lib/hooks/tests/hooks_manager_unittest.cc
index 72e475d776..952c67c542 100644
--- a/src/lib/hooks/tests/hooks_manager_unittest.cc
+++ b/src/lib/hooks/tests/hooks_manager_unittest.cc
@@ -431,7 +431,7 @@ TEST_F(HooksManagerTest, PrePostCalloutShared) {
 
     HooksManager::callCallouts(hookpt_two_index_, *handle);
 
-    // Expect same value i.e. 1027 * 2 
+    // Expect same value i.e. 1027 * 2
     result = 0;
     handle->getArgument("result", result);
     EXPECT_EQ(2054, result);
@@ -564,8 +564,13 @@ TEST_F(HooksManagerTest, RegisterHooks) {
     EXPECT_EQ(2, HooksManager::registerHook(string("alpha")));
     EXPECT_EQ(3, HooksManager::registerHook(string("beta")));
     EXPECT_EQ(4, HooksManager::registerHook(string("gamma")));
-    EXPECT_THROW(static_cast<void>(HooksManager::registerHook(string("alpha"))),
-                 DuplicateHook);
+
+
+    // The code used to throw, but it now allows to register the same
+    // hook several times. It simply returns existing index.
+    //EXPECT_THROW(static_cast<void>(HooksManager::registerHook(string("alpha"))),
+    //             DuplicateHook);
+    EXPECT_EQ(2, HooksManager::registerHook(string("alpha")));
 
     // ... an check the hooks are as we expect.
     EXPECT_EQ(5, ServerHooks::getServerHooks().getCount());
diff --git a/src/lib/hooks/tests/server_hooks_unittest.cc b/src/lib/hooks/tests/server_hooks_unittest.cc
index d66966af09..f2c4979f57 100644
--- a/src/lib/hooks/tests/server_hooks_unittest.cc
+++ b/src/lib/hooks/tests/server_hooks_unittest.cc
@@ -54,8 +54,9 @@ TEST(ServerHooksTest, RegisterHooks) {
 }
 
 // Check that duplicate names cannot be registered.
-
-TEST(ServerHooksTest, DuplicateHooks) {
+// This test has been updated. See #5251 for details. The old
+// code is retained in case we decide to get back to it.
+TEST(ServerHooksTest, DISABLED_OldDuplicateHooks) {
     ServerHooks& hooks = ServerHooks::getServerHooks();
     hooks.reset();
 
@@ -68,6 +69,29 @@ TEST(ServerHooksTest, DuplicateHooks) {
     EXPECT_THROW(hooks.registerHook("gamma"), DuplicateHook);
 }
 
+// Check that duplicate names are handled properly. The code used to throw,
+// but it now returns the existing index. See #5251 for details.
+TEST(ServerHooksTest, NewDuplicateHooks) {
+    ServerHooks& hooks = ServerHooks::getServerHooks();
+    hooks.reset();
+
+    int index = hooks.getIndex("context_create");
+
+    // Ensure we can duplicate one of the existing names.
+    // Instead of throwing, we just check that a resonable
+    // index has been returned.
+    EXPECT_EQ(index, hooks.registerHook("context_create"));
+
+    // Check that mutiple attempts to register the same hook will return
+    // existing index.
+    int gamma = hooks.registerHook("gamma");
+    EXPECT_EQ(2, gamma);
+    EXPECT_EQ(gamma, hooks.registerHook("gamma"));
+    EXPECT_EQ(gamma, hooks.registerHook("gamma"));
+    EXPECT_EQ(gamma, hooks.registerHook("gamma"));
+    EXPECT_EQ(gamma, hooks.registerHook("gamma"));
+}
+
 // Checks that we can get the name of the hooks.
 
 TEST(ServerHooksTest, GetHookNames) {