Merge branch 'work/abortsocket'

src/lib/server_common/portconfig.cc

@@ -64,8 +64,7 @@ parseAddresses(isc::data::ConstElementPtr addresses,
                     }
                     result.push_back(AddressPair(addr->stringValue(),
                         port->intValue()));
-                }
-                catch (const TypeError&) { // Better error message
+                } catch (const TypeError&) { // Better error message
                     LOG_ERROR(logger, SRVCOMM_ADDRESS_TYPE).
                         arg(addrPair->str());
                     isc_throw(TypeError,
@@ -135,8 +134,7 @@ installListenAddresses(const AddressList& newAddresses,
         }
         setAddresses(service, newAddresses);
         addressStore = newAddresses;
-    }
-    catch (const exception& e) {
+    } catch (const SocketRequestor::NonFatalSocketError& e) {
         /*
          * If one of the addresses isn't set successfully, we will restore
          * the old addresses, the behavior is that either all address are
@@ -153,7 +151,7 @@ installListenAddresses(const AddressList& newAddresses,
         LOG_ERROR(logger, SRVCOMM_ADDRESS_FAIL).arg(e.what());
         try {
             setAddresses(service, addressStore);
-        } catch (const exception& e2) {
+        } catch (const SocketRequestor::NonFatalSocketError& e2) {
             LOG_FATAL(logger, SRVCOMM_ADDRESS_UNRECOVERABLE).arg(e2.what());
             // If we can't set the new ones, nor the old ones, at least
             // releasing everything should work. If it doesn't, there isn't
@@ -164,6 +162,16 @@ installListenAddresses(const AddressList& newAddresses,
         //Anyway the new configure has problem, we need to notify configure
         //manager the new configure doesn't work
         throw;
+    } catch (const exception& e) {
+        // Any other kind of exception is fatal. It might mean we are in
+        // inconsistent state with the boss/socket creator, so we abort
+        // to make sure it doesn't last.
+        LOG_FATAL(logger, SRVCOMM_EXCEPTION_ALLOC).arg(e.what());
+        abort();
+    } catch (...) {
+        // As the previous one, but we know even less info
+        LOG_FATAL(logger, SRVCOMM_UNKNOWN_EXCEPTION_ALLOC);
+        abort();
     }
 }
 

src/lib/server_common/server_common_messages.mes

@@ -78,6 +78,22 @@ addresses we are going to listen on (eg. there will be one log message
 per pair). This appears only after SRVCOMM_SET_LISTEN, but might
 be hidden, as it has higher debug level.
 
+% SRVCOMM_EXCEPTION_ALLOC exception when allocating a socket: %1
+The process tried to allocate a socket using the socket creator, but an error
+occurred. But it is not one of the errors we are sure are "safe". In this case
+it is unclear if the unsuccessful communication left the process and the bind10
+process in inconsistent state, so the process is going to abort to prevent
+further problems in that area.
+
+This is probably a bug in the code, but it could be caused by other unusual
+conditions (like insufficient memory, deleted socket file used for
+communication).
+
+% SRVCOMM_UNKNOWN_EXCEPTION_ALLOC unknown exception when allocating a socket
+The situation is the same as in the SRVCOMM_EXCEPTION_ALLOC case, but further
+details about the error are unknown, because it was signaled by throwing
+something not being an exception. This is definitely a bug.
+
 % SRVCOMM_KEYS_DEINIT deinitializing TSIG keyring
 Debug message indicating that the server is deinitializing the TSIG keyring.
 

src/lib/server_common/tests/portconfig_unittest.cc

@@ -265,7 +265,7 @@ TEST_F(InstallListenAddresses, brokenRollback) {
     sock_requestor_.given_tokens_.clear();
     sock_requestor_.break_rollback_ = true;
     EXPECT_THROW(installListenAddresses(invalid_, store_, dnss_),
-                 SocketRequestor::SocketError);
+                 SocketRequestor::NonFatalSocketError);
     // No addresses here
     EXPECT_TRUE(store_.empty());
     // The first pair should be requested in the first part of the failure to
@@ -295,4 +295,46 @@ TEST_F(InstallListenAddresses, brokenRollback) {
                                 "released");
 }
 
+// Make sure the death tests are filterable away.
+typedef InstallListenAddresses InstallListenAddressesDeathTest;
+
+// We make the socket requestor throw a "fatal" exception, one where we can't be
+// sure the state between processes is consistent. So we abort in that case.
+TEST_F(InstallListenAddressesDeathTest, inconsistent) {
+    AddressList deathAddresses;
+    deathAddresses.push_back(AddressPair("192.0.2.3", 5288));
+    // Make sure it actually kills the application (there should be an abort
+    // in this case)
+    EXPECT_DEATH({
+                    try {
+                        installListenAddresses(deathAddresses, store_, dnss_);
+                    } catch (...) {
+                        // Prevent exceptions killing the application, we need
+                        // to make sure it dies the real hard way
+                    };
+                 }, "");
+}
+
+// If we are unable to tell the boss we closed a socket, we abort, as we are
+// not consistent with the boss most probably.
+TEST_F(InstallListenAddressesDeathTest, cantClose) {
+    installListenAddresses(valid_, store_, dnss_);
+    AddressList empty;
+    // Instruct it to fail on close
+    sock_requestor_.break_release_ = true;
+    EXPECT_DEATH({
+                    try {
+                        // Setting to empty will close all current sockets.
+                        // And thanks to the break_release_, the close will
+                        // throw, which will make it crash.
+                        installListenAddresses(empty, store_, dnss_);
+                    } catch (...) {
+                        // To make sure it is killed by abort, not by some
+                        // (unhandled) exception
+                    };
+                }, "");
+    // And reset it back, so it can safely clean up itself.
+    sock_requestor_.break_release_ = false;
+}
+
 }

src/lib/testutils/socket_request.h

@@ -62,8 +62,8 @@ public:
     TestSocketRequestor(asiodns::DNSService& dnss,
                         server_common::portconfig::AddressList& store,
                         uint16_t expect_port) :
-        last_token_(0), break_rollback_(false), dnss_(dnss), store_(store),
-        expect_port_(expect_port)
+        last_token_(0), break_rollback_(false), break_release_(false),
+        dnss_(dnss), store_(store), expect_port_(expect_port)
     {
         // Prepare the requestor (us) for the test
         server_common::initTestSocketRequestor(this);
@@ -106,11 +106,23 @@ public:
     /// ::1 address is requested.
     bool break_rollback_;
 
+    /// \brief Throw on releaseSocket
+    ///
+    /// If this is set to true, the releaseSocket will throw SocketError.
+    /// Defaults to false.
+    bool break_release_;
+
     /// \brief Release a socket
     ///
     /// This only stores the token passed.
     /// \param token The socket to release
+    ///
+    /// \throw SocketError in case the break_release_ is set to true. This is
+    ///     to test exception handling.
     void releaseSocket(const std::string& token) {
+        if (break_release_) {
+            isc_throw(SocketError, "Fatal test socket error");
+        }
         released_tokens_.push_back(token);
     }
 
@@ -119,8 +131,9 @@ public:
     /// This creates a new token and fakes a new socket and returns it.
     /// The token is stored.
     ///
-    /// In case the address is 192.0.2.2 or if the break_rollback_ is true
-    /// and address is ::1, it throws.
+    /// In case the address is 192.0.2.2, it throws SocketAllocateError
+    /// or if the break_rollback_ is true and address is ::1, it throws
+    /// ShareError. If the address is 192.0.2.3, it throws SocketError.
     ///
     /// The tokens produced are in form of protocol:address:port:fd. The fds
     /// start at 1 and increase by each successfull call.
@@ -131,13 +144,18 @@ public:
     /// \param mode checked to be DONT_SHARE for now
     /// \param name checked to be dummy_app for now
     /// \return The token and FD
+    /// \throw SocketAllocateError as described above, to test error handling
+    /// \throw ShareError as described above, to test error handling
     /// \throw SocketError as described above, to test error handling
     SocketID requestSocket(Protocol protocol, const std::string& address,
                            uint16_t port, ShareMode mode,
                            const std::string& name)
     {
         if (address == "192.0.2.2") {
-            isc_throw(SocketError, "This address is not allowed");
+            isc_throw(SocketAllocateError, "This address is not allowed");
+        }
+        if (address == "192.0.2.3") {
+            isc_throw(SocketError, "Fatal test error");
         }
         if (address == "::1" && break_rollback_) {
             // This is valid address, but in case we need to break the
@@ -145,7 +163,7 @@ public:
             //
             // We break the second address to see the first one was
             // allocated and then returned
-            isc_throw(SocketError,
+            isc_throw(ShareError,
                       "This address is available, but not for you");
         }
         const std::string proto(protocol == TCP ? "TCP" : "UDP");