[3405] Globally check for duplicated signals in SignalSets.

src/lib/util/io/signal_set.cc

@@ -21,9 +21,25 @@ using namespace isc::util::io;
 
 namespace {
 
-/// @brief Returns a pointer to static collection of signals.
+/// @brief Returns a pointer to the global set of registered signals.
 ///
-/// @return Static collection of signals.
+/// Multiple instances of @c SignalSet may use this pointer to access
+/// and update the set.
+///
+/// @return Pointer to the global set of registered signals. This pointer
+/// is always initialized and points to a valid object.
+std::set<int>* getRegisteredSignals() {
+    static std::set<int> registered_signals;
+    return (&registered_signals);
+}
+
+/// @brief Returns a pointer to static collection of signals received.
+///
+/// Multiple instances of @c SignalSet may use this pointer to access
+/// and update the queue of signals received.
+///
+/// @return Static collection of signals received. This pointer is always
+/// initialized and points to a valid object.
 std::list<int>* getSignalStates() {
     static std::list<int> states;
     return (&states);
@@ -84,20 +100,22 @@ SignalSet::SignalSet(const int sig0, const int sig1, const int sig2) {
 
 SignalSet::~SignalSet() {
     // Set default signal handlers.
-    clear();
+    try {
+        clear();
+    } catch (...) {
+        // Not a good thing to throw from a destructor. in fact this should
+        // not throw an exception because we just unregister the signals
+        // that we have previously registered. So the signal codes are fine.
+    }
 }
 
 void
 SignalSet::add(const int sig) {
-    std::pair<Pool::iterator, bool> ret = registered_signals_.insert(sig);
-    if (!ret.second) {
-        isc_throw(SignalSetError, "attempt to register a duplicate signal "
-                  << sig);
-    }
+    insert(sig);
     struct sigaction sa;
     sa.sa_handler = internalHandler;
     if (sigaction(sig, &sa, 0) < 0) {
-        registered_signals_.erase(sig);
+        erase(sig);
         isc_throw(SignalSetError, "failed to register a signal handler for"
                   " signal " << sig << ": " << strerror(errno));
     }
@@ -105,8 +123,12 @@ SignalSet::add(const int sig) {
 
 void
 SignalSet::clear() {
-    Pool all_signals = registered_signals_;
-    for (Pool::const_iterator it = all_signals.begin();
+    // Iterate over a copy of the registered signal set because the
+    // remove function is erasing the elements and we don't want to
+    // erase the elements we are iterating over. This would cause
+    // a segfault.
+    std::set<int> all_signals = local_signals_;
+    for (std::set<int>::const_iterator it = all_signals.begin();
          it != all_signals.end(); ++it) {
         remove(*it);
     }
@@ -122,6 +144,17 @@ SignalSet::getNext() const {
 }
 
 void
+SignalSet::erase(const int sig) {
+    if (local_signals_.find(sig) == local_signals_.end()) {
+        isc_throw(SignalSetError, "failed to unregister signal " << sig
+                  << " from a signal set: signal is not owned by the"
+                  " signal set");
+    }
+    getRegisteredSignals()->erase(sig);
+    local_signals_.erase(sig);
+}
+
+void
 SignalSet::handleNext(SignalHandler signal_handler) {
     block();
     int signum = getNext();
@@ -138,10 +171,22 @@ SignalSet::handleNext(SignalHandler signal_handler) {
 }
 
 void
+SignalSet::insert(const int sig) {
+    std::set<int>* global_signals = getRegisteredSignals();
+    if ((global_signals->find(sig) != global_signals->end()) ||
+        (local_signals_.find(sig) != local_signals_.end())) {
+        isc_throw(SignalSetError, "attempt to register a duplicate signal "
+                  << sig);
+    }
+    global_signals->insert(sig);
+    local_signals_.insert(sig);
+}
+
+void
 SignalSet::maskSignals(const int mask) const {
     sigset_t new_set;
-    for (Pool::const_iterator it = registered_signals_.begin();
-         it != registered_signals_.end(); ++it) {
+    for (std::set<int>::const_iterator it = getRegisteredSignals()->begin();
+         it != getRegisteredSignals()->end(); ++it) {
         sigaddset(&new_set, *it);
     }
     sigprocmask(mask, &new_set, 0);
@@ -149,14 +194,18 @@ SignalSet::maskSignals(const int mask) const {
 
 void
 SignalSet::remove(const int sig) {
-    if (registered_signals_.find(sig) != registered_signals_.end()) {
+    // Unregister only if we own this signal.
+    if (local_signals_.find(sig) != local_signals_.end()) {
         struct sigaction sa;
         sa.sa_handler = SIG_DFL;
         if (sigaction(sig, &sa, 0) < 0) {
             isc_throw(SignalSetError, "unable to restore original signal"
                       " handler for signal: " << sig);
         }
-        registered_signals_.erase(sig);
+        erase(sig);
+    } else {
+        isc_throw(SignalSetError, "failed to unregister signal " << sig
+                  << ": this signal is not owned by the signal set");
     }
 }
 

src/lib/util/io/signal_set.h

@@ -53,11 +53,11 @@ typedef boost::function<void(int signum)> SignalHandler;
 /// spent on handling the signal and process interruption. The process
 /// can later check the signals received and call the handlers on its
 /// descretion by calling a @c isc::util::io::SignalSet::handleNext function.
+///
+/// @note This class is not thread safe. It uses static variables and
+/// functions to track a global state of signal registration and received
+/// signals' queue.
 class SignalSet : public boost::noncopyable {
-private:
-    /// @brief Defines a type representing a collection of signals.
-    typedef std::set<int> Pool;
-
 public:
 
     /// @brief Constructor installing one signal.
@@ -84,6 +84,11 @@ public:
     /// the signal is invalid.
     SignalSet(const int sig0, const int sig1, const int sig2);
 
+    /// @brief Destructor.
+    ///
+    /// Removes installed handlers.
+    ~SignalSet();
+
     /// @brief Installs the handler for the specified signal.
     ///
     /// This function adds a signal to the set. When the signal is received
@@ -132,6 +137,22 @@ private:
         maskSignals(SIG_BLOCK);
     }
 
+    /// @brief Removes the signal from the set.
+    ///
+    /// This function removes only a signal which is owned by this signal set.
+    ///
+    /// @param sig Signal to be removed.
+    /// @throw SignalSetError if the signal being removed is not owned by this
+    /// signal set.
+    void erase(const int sig);
+
+    /// @brief Insert a signal to the set.
+    ///
+    /// @param sig Signal to be inserted.
+    /// @throw SignalSetError if a signal being inserted has already been
+    /// registered in this or other signal set.
+    void insert(const int sig);
+
     /// @brief Applies a mask to all signals in the set.
     ///
     /// This function is used by @c SignalSet::block and @c SignalSet::unblock
@@ -147,8 +168,8 @@ private:
         maskSignals(SIG_UNBLOCK);
     }
 
-    /// @brief Holds a collection of installed signals.
-    Pool registered_signals_;
+    /// @brief Stores the set of signals registered in this signal set.
+    std::set<int> local_signals_;
 };
 
 }

src/lib/util/io/tests/signal_set_unittest.cc

@@ -26,7 +26,8 @@ using namespace isc::util::io;
 class SignalSetTest : public ::testing::Test {
 public:
 
-    SignalSetTest() {
+    SignalSetTest()
+        : signal_set_() {
         handler_calls_ = 0;
         signum_ = -1;
     }
@@ -48,13 +49,14 @@ public:
 
     static int handler_calls_;
     static int signum_;
-    boost::shared_ptr<SignalSet> signal_set_;
+    SignalSetPtr signal_set_;
 
 };
 
 int SignalSetTest::handler_calls_ = 0;
 int SignalSetTest::signum_ = -1;
 
+/// Check that the signals are recorded by the signal handlers.
 TEST_F(SignalSetTest, twoSignals) {
     // Register handlers for two signals.
     signal_set_.reset(new SignalSet(SIGHUP, SIGINT));
@@ -92,5 +94,46 @@ TEST_F(SignalSetTest, twoSignals) {
     EXPECT_NO_THROW(signal_set_->remove(SIGINT));
 }
 
+// Check that each signal set removes only the signals that it has been used
+// to register.
+TEST_F(SignalSetTest, twoSignalSets) {
+    // Register handlers for SIGHUP using one signal set.
+    ASSERT_NO_THROW(signal_set_.reset(new SignalSet(SIGHUP)));
+    // Define another signal set and register a different signal.
+    SignalSetPtr other_set;
+    ASSERT_NO_THROW(other_set.reset(new SignalSet(SIGINT)));
+    // The SIGHUP has been already registsred with the other signal
+    // set, so it should not be possible to register it again.
+    ASSERT_THROW(other_set->add(SIGHUP), SignalSetError);
+    // It shouldn't be possible to remove the signal registered in a different
+    // signal set.
+    ASSERT_THROW(other_set->remove(SIGHUP), SignalSetError);
+    // Remove all signals from the first signal set. The signals registered
+    // with the other signal signal set should be preserved.
+    ASSERT_NO_THROW(signal_set_->clear());
+    // Check indirectly that the SIGINT is still registered. An attempt to
+    // register registered signal should result in failure.
+    EXPECT_THROW(other_set->add(SIGINT), SignalSetError);
+    // But, we should be able to regsiter SIGHUP.
+    EXPECT_NO_THROW(other_set->add(SIGHUP));
+}
+
+/// Check that it is not possible to duplicate signals.
+TEST_F(SignalSetTest, duplicates) {
+    ASSERT_NO_THROW(signal_set_.reset(new SignalSet(SIGHUP)));
+    // It shouldn't be possible to register the same signal.
+    EXPECT_THROW(signal_set_->add(SIGHUP), SignalSetError);
+    // But ok to register a different one.
+    EXPECT_NO_THROW(signal_set_->add(SIGTERM));
+    // Now, let's define other signal set.
+    SignalSetPtr other;
+    // SIGINT hasn't been registered in the first signal set
+    // so it should be fine to register.
+    ASSERT_NO_THROW(other.reset(new SignalSet(SIGINT)));
+    // SIGHUP has been already registered in the first signal set so
+    // an attempt to register it again should result in a failure.
+    EXPECT_THROW(other->add(SIGHUP), SignalSetError);
+}
+
 
 } // end of anonymous namespace