From: Michal 'vorner' Vaner Date: Fri, 27 Jan 2012 18:31:36 +0000 (+0100) Subject: [1543] Abort on problem requesting a socket X-Git-Tag: trac2351_base~273^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b0d52a6010210282915fa8defa027b6cf1f0e36b;p=thirdparty%2Fkea.git [1543] Abort on problem requesting a socket If it is not one of the "known to be OK" exceptions, we abort. --- diff --git a/src/lib/server_common/portconfig.cc b/src/lib/server_common/portconfig.cc index 556f0da75c..62e291cb07 100644 --- a/src/lib/server_common/portconfig.cc +++ b/src/lib/server_common/portconfig.cc @@ -136,7 +136,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 +153,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 @@ -165,6 +165,18 @@ installListenAddresses(const AddressList& newAddresses, //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(); + } } } diff --git a/src/lib/server_common/server_common_messages.mes b/src/lib/server_common/server_common_messages.mes index 0efe38851f..eb6362d06a 100644 --- a/src/lib/server_common/server_common_messages.mes +++ b/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 us and the bind10 process +in inconsistent state, so we're 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 we don't +know further details about the error, 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. diff --git a/src/lib/server_common/tests/portconfig_unittest.cc b/src/lib/server_common/tests/portconfig_unittest.cc index ee17e56a23..753e808f10 100644 --- a/src/lib/server_common/tests/portconfig_unittest.cc +++ b/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,24 @@ 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_); + } + // Prevent exceptions killing the application, we need to + // make sure it dies the real hard way + catch (...) {}; + }, ""); +} + } diff --git a/src/lib/testutils/socket_request.h b/src/lib/testutils/socket_request.h index c06c969673..89279de6f3 100644 --- a/src/lib/testutils/socket_request.h +++ b/src/lib/testutils/socket_request.h @@ -119,8 +119,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 +132,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 +151,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");