[master] Merge branch 'trac4000' (signal handler should not modify errno)

src/bin/dhcp4/dhcp4_messages.mes

@@ -201,6 +201,10 @@ from a client. Server does not process empty Hostname options and therefore
 option is skipped. The argument holds the client and transaction identification
 information.
 
+% DHCP4_HANDLE_SIGNAL_EXCEPTION An exception was thrown while handing signal: %1
+This error message is printed when an ISC or standard exception was raised during signal
+processing. This likely indicates a coding error and should be reported to ISC.
+
 % DHCP4_HOOKS_LIBS_RELOAD_FAIL reload of hooks libraries failed
 A "libreload" command was issued to reload the hooks libraries but for
 some reason the reload failed.  Other error messages issued from the

src/bin/dhcp4/dhcp4_srv.cc

@@ -400,8 +400,8 @@ Dhcpv4Srv::run() {
         } catch (const SignalInterruptOnSelect) {
             // Packet reception interrupted because a signal has been received.
             // This is not an error because we might have received a SIGTERM,
-            // SIGINT or SIGHUP which are handled by the server. For signals
+            // SIGINT, SIGHUP or SIGCHILD which are handled by the server. For
-            // that are not handled by the server we rely on the default
+            // signals that are not handled by the server we rely on the default
             // behavior of the system.
             LOG_DEBUG(packet4_logger, DBG_DHCP4_DETAIL, DHCP4_BUFFER_WAIT_SIGNAL)
                 .arg(signal_set_->getNext());
@@ -420,7 +420,14 @@ Dhcpv4Srv::run() {
         // select() function is called. If the function was called before
         // receivePacket the process could wait up to the duration of timeout
         // of select() to terminate.
+        try {
             handleSignal();
+        } catch (const std::exception& e) {
+            // Standard exception occurred. Let's be on the safe side to
+            // catch std::exception.
+            LOG_ERROR(dhcp4_logger, DHCP4_HANDLE_SIGNAL_EXCEPTION)
+                .arg(e.what());
+        }
 
         // Execute ready timers for the lease database, e.g. Lease File Cleanup.
         try {
@@ -601,11 +608,12 @@ Dhcpv4Srv::run() {
                 // "switch" statement.
                 ;
             }
-        } catch (const isc::Exception& e) {
+        } catch (const std::exception& e) {
 
-            // Catch-all exception (at least for ones based on the isc Exception
+            // Catch-all exception (we used to call only isc::Exception, but
-            // class, which covers more or less all that are explicitly raised
+            // std::exception could potentially be raised and if we don't catch
-            // in the Kea code).  Just log the problem and ignore the packet.
+            // it here, it would be caught in main() and the process would
+            // terminate).  Just log the problem and ignore the packet.
             // (The problem is logged as a debug message because debug is
             // disabled by default - it prevents a DDOS attack based on the
             // sending of problem packets.)

src/bin/dhcp6/dhcp6_messages.mes

@@ -242,6 +242,10 @@ probable if you see many such messages. Clients will recover from this,
 but they will most likely get a different IP addresses and experience
 a brief service interruption.
 
+% DHCP6_HANDLE_SIGNAL_EXCEPTION An exception was thrown while handing signal: %1
+This error message is printed when an exception was raised during signal
+processing. This likely indicates a coding error and should be reported to ISC.
+
 % DHCP6_HOOKS_LIBS_RELOAD_FAIL reload of hooks libraries failed
 A "libreload" command was issued to reload the hooks libraries but for
 some reason the reload failed.  Other error messages issued from the

src/bin/dhcp6/dhcp6_srv.cc

@@ -382,7 +382,13 @@ bool Dhcpv6Srv::run() {
         // is called. If the function was called before receivePacket the
         // process could wait up to the duration of timeout of select() to
         // terminate.
+        try {
             handleSignal();
+        } catch (const std::exception& e) {
+            // An (a standard or ISC) exception occurred.
+            LOG_ERROR(dhcp6_logger, DHCP6_HANDLE_SIGNAL_EXCEPTION)
+                .arg(e.what());
+        }
 
         // Execute ready timers for the lease database, e.g. Lease File Cleanup.
         try {
@@ -585,11 +591,12 @@ bool Dhcpv6Srv::run() {
             // Increase the statistic of dropped packets.
             StatsMgr::instance().addValue("pkt6-receive-drop", static_cast<int64_t>(1));
 
-        } catch (const isc::Exception& e) {
+        } catch (const std::exception& e) {
 
             // Catch-all exception (at least for ones based on the isc Exception
             // class, which covers more or less all that are explicitly raised
-            // in the Kea code).  Just log the problem and ignore the packet.
+            // in the Kea code), but also the standard one, which may possibly be
+            // thrown from boost code.  Just log the problem and ignore the packet.
             // (The problem is logged as a debug message because debug is
             // disabled by default - it prevents a DDOS attack based on the
             // sending of problem packets.)

src/lib/exceptions/exceptions.cc

@@ -22,18 +22,30 @@ namespace isc {
 
 const char*
 Exception::what() const throw() {
+    return (what(false));
+}
+
+const char*
+Exception::what(bool verbose) const throw() {
+
     const char* whatstr = "isc::Exception";
 
-    // XXX: even though it's very unlikely that c_str() throws an exception,
+    // XXX: Even though it's very unlikely that c_str() throws an exception,
     // it's still not 100% guaranteed.  To meet the exception specification
     // of this function, we catch any unexpected exception and fall back to
     // the pre-defined constant.
     try {
+        if (verbose) {
+            static std::stringstream location;
+            location.str("");
+            location << what_ << "[" << file_ << ":" << line_ << "]";
+            whatstr = location.str().c_str();
+        } else {
             whatstr = what_.c_str();
+        }
     } catch (...) {
         // no exception handling is necessary.  just have to catch exceptions.
     }
-
     return (whatstr);
 }
 

src/lib/exceptions/exceptions.h

@@ -73,6 +73,16 @@ public:
     ///
     /// @return A C-style character string of the exception cause.
     virtual const char* what() const throw();
+
+    /// \brief Returns a C-style charater string of the cause of exception.
+    ///
+    /// With verbose set to true, also returns file name and line numbers.
+    /// Note that we can't simply define a single what() method with parameters,
+    /// as the compiler would complain that it shadows the base class method.
+    ///
+    /// \param verbose if set to true, filename and line number will be added.
+    /// \return A C-style character string of the exception cause.
+    virtual const char* what(bool verbose) const throw();
     //@}
 
     ///

src/lib/exceptions/tests/exceptions_unittest.cc

@@ -16,6 +16,7 @@
 #include <string>
 
 #include <exceptions/exceptions.h>
+#include <sstream>
 
 #include <gtest/gtest.h>
 
@@ -47,4 +48,24 @@ TEST_F(ExceptionTest, stdInheritance) {
         EXPECT_EQ(std::string(ex.what()), std::string(teststring));
     }
 }
+
+// Tests whether verbose is disabled by default and can be enabled, if
+// needed.
+TEST_F(ExceptionTest, verbose) {
+
+    // This code is line numbers sensitive. Make sure no edits are done between
+    // this line and isc_throw below. Update that +3 offset, if needed.
+    std::stringstream expected;
+    expected << teststring << "[" << std::string(__FILE__)
+             << ":" << int(__LINE__ + 3) << "]";
+
+    try {
+        isc_throw(Exception, teststring);
+    } catch (const isc::Exception& ex) {
+        EXPECT_EQ(std::string(ex.what()), std::string(teststring));
+        EXPECT_EQ(std::string(ex.what(false)), std::string(teststring));
+        EXPECT_EQ(expected.str(), std::string(ex.what(true)));
+    }
+
+}
 }

src/lib/util/process_spawn.cc

@@ -19,6 +19,7 @@
 #include <map>
 #include <signal.h>
 #include <stdlib.h>
+#include <errno.h>
 #include <unistd.h>
 #include <sys/wait.h>
 
@@ -279,6 +280,11 @@ ProcessSpawnImpl::waitForProcess(int signum) {
     if (signum != SIGCHLD) {
         return (false);
     }
+
+    // Need to store current value of errno, so we could restore it
+    // after this signal handler does his work.
+    int errno_value = errno;
+
     for (;;) {
         int status = 0;
         pid_t pid = waitpid(-1, &status, WNOHANG);
@@ -294,6 +300,16 @@ ProcessSpawnImpl::waitForProcess(int signum) {
             proc->second.running_ = false;
         }
     }
+
+    // Need to restore previous value of errno. We called waitpid(),
+    // which likely indicated its result by setting errno to ECHILD.
+    // This is a signal handler, which can be called while virtually
+    // any other code being run. If we're unlucky, we could receive a
+    // signal when running a code that is about to check errno. As a
+    // result the code would detect errno=ECHILD in places which are
+    // completely unrelated to child or processes in general.
+    errno = errno_value;
+
     return (true);
 }
 

src/lib/util/tests/process_spawn_unittest.cc

@@ -16,6 +16,7 @@
 #include <gtest/gtest.h>
 #include <signal.h>
 #include <stdint.h>
+#include <errno.h>
 #include <sys/types.h>
 #include <unistd.h>
 
@@ -36,24 +37,67 @@ std::string getApp() {
     return (s.str());
 }
 
-/// @brief Waits for the specified process to finish.
+/// @brief Waits for the specified process to finish (using sleep)
 ///
 /// @param process An object which started the process.
 /// @param pid ID of the spawned process.
 /// @param timeout Timeout in seconds.
+/// @param sleep specifies whether usleep(1ms) should be used or not.
+///
+/// This method uses usleep() to wait between checks. For an alternative,
+/// see waitForProcessFast(). Note: If the signal comes in when the
+/// loop calls usleep(), usleep() will be aborted and errno will be set
+/// to EINTR to indicate that interruption. Therefore this method is not
+/// suitable for tests that want to observe errno value. See
+/// @ref waitForProcessFast as an alternative.
 ///
 /// @return true if the process ended, false otherwise
 bool waitForProcess(const ProcessSpawn& process, const pid_t pid,
                     const uint8_t timeout) {
     uint32_t iterations = 0;
     const uint32_t iterations_max = timeout * 1000;
+
+    int errno_save = errno;
     while (process.isRunning(pid) && (iterations < iterations_max)) {
         usleep(1000);
         ++iterations;
     }
+    errno = errno_save;
     return (iterations < iterations_max);
 }
 
+/// @brief Waits for the specified process to finish (using fast loop)
+///
+/// @param process An object which started the process.
+/// @param pid ID of the spawned process.
+/// @param timeout Timeout in seconds.
+/// @param sleep specifies whether usleep(1ms) should be used or not.
+///
+/// This method does not use any sleep() calls, but rather iterates through
+/// the loop very fast. This is not recommended in general, but is necessary
+/// to avoid updating errno by sleep() after receving a signal.
+///
+/// Note: the timeout is only loosely accurate. Depending on the fraction
+/// of second it was started on, it may terminate later by up to almost
+/// whole second.
+///
+/// @return true if the process ended, false otherwise
+bool waitForProcessFast(const ProcessSpawn& process, const pid_t pid,
+                        const uint8_t timeout) {
+    time_t before = time(NULL);
+    while (process.isRunning(pid)) {
+        time_t now = time(NULL);
+
+        // The difference before we started and now is greater than
+        // the timeout, we should give up.
+        if (now - before > timeout) {
+            return (false);
+        }
+    }
+
+    return (true);
+}
+
 // This test verifies that the external application can be ran with
 // arguments and that the exit code is gathered.
 TEST(ProcessSpawn, spawnWithArgs) {
@@ -169,4 +213,30 @@ TEST(ProcessSpawn, isRunning) {
     ASSERT_TRUE(waitForProcess(process, pid, 2));
 }
 
+// This test verifies that the signal handler does not modify value of
+// errno.
+TEST(ProcessSpawn, errnoInvariance) {
+
+    // Set errno to an arbitrary value. We'll later check that it was not
+    // stumped on.
+    errno = 123;
+
+    std::vector<std::string> args;
+    args.push_back("-e");
+    args.push_back("64");
+    ProcessSpawn process(getApp(), args);
+    pid_t pid = 0;
+    ASSERT_NO_THROW(pid = process.spawn());
+
+    ASSERT_TRUE(waitForProcessFast(process, pid, 2));
+
+    EXPECT_EQ(64, process.getExitStatus(pid));
+
+    // errno value should be set to be preserved, despite the SIGCHILD
+    // handler (ProcessSpawnImpl::waitForProcess) calling waitpid(), which
+    // will likely set errno to ECHILD. See trac4000.
+    EXPECT_EQ(123, errno);
+}
+
+
 } // end of anonymous namespace