face: improve error handling in UnixStreamChannel
Change-Id: I5d37b56e74264490089a4c18f527bbcc202a480d
diff --git a/daemon/face/unix-stream-channel.cpp b/daemon/face/unix-stream-channel.cpp
index 150fad4..a1abe9c 100644
--- a/daemon/face/unix-stream-channel.cpp
+++ b/daemon/face/unix-stream-channel.cpp
@@ -30,7 +30,6 @@
#include "common/global.hpp"
#include <boost/filesystem.hpp>
-#include <sys/stat.h> // for chmod()
namespace nfd::face {
@@ -50,10 +49,10 @@
{
if (isListening()) {
// use the non-throwing variants during destruction and ignore any errors
- boost::system::error_code error;
- m_acceptor.close(error);
+ boost::system::error_code ec;
+ m_acceptor.close(ec);
NFD_LOG_CHAN_TRACE("Removing socket file");
- boost::filesystem::remove(m_endpoint.path(), error);
+ boost::filesystem::remove(m_endpoint.path(), ec);
}
}
@@ -70,42 +69,56 @@
namespace fs = boost::filesystem;
fs::path socketPath = m_endpoint.path();
- fs::file_type type = fs::symlink_status(socketPath).type();
-
- if (type == fs::socket_file) {
- boost::system::error_code error;
- boost::asio::local::stream_protocol::socket socket(getGlobalIoService());
- socket.connect(m_endpoint, error);
- NFD_LOG_CHAN_TRACE("connect() on existing socket file returned: " << error.message());
- if (!error) {
- // someone answered, leave the socket alone
- NDN_THROW(Error("Socket file at " + m_endpoint.path() + " belongs to another process"));
- }
- else if (error == boost::asio::error::connection_refused ||
- error == boost::asio::error::timed_out) {
- // no one is listening on the remote side,
- // we can safely remove the stale socket
- NFD_LOG_CHAN_DEBUG("Removing stale socket file");
- fs::remove(socketPath);
- }
- }
- else if (type != fs::file_not_found) {
- NDN_THROW(Error(m_endpoint.path() + " already exists and is not a socket file"));
- }
-
- // ensure parent directory exists before creating socket
+ // ensure parent directory exists
fs::path parent = socketPath.parent_path();
if (!parent.empty() && fs::create_directories(parent)) {
NFD_LOG_CHAN_TRACE("Created directory " << parent);
}
- m_acceptor.open();
- m_acceptor.bind(m_endpoint);
- m_acceptor.listen(backlog);
-
- if (::chmod(m_endpoint.path().data(), 0666) < 0) {
- NDN_THROW_ERRNO(Error("Failed to chmod " + m_endpoint.path()));
+ boost::system::error_code ec;
+ fs::file_type type = fs::symlink_status(socketPath).type();
+ if (type == fs::socket_file) {
+ // if the socket file already exists, there may be another instance
+ // of NFD running on the system: make sure we don't steal its socket
+ boost::asio::local::stream_protocol::socket socket(getGlobalIoService());
+ socket.connect(m_endpoint, ec);
+ NFD_LOG_CHAN_TRACE("connect() on existing socket file returned: " << ec.message());
+ if (!ec) {
+ // someone answered, leave the socket alone
+ ec = boost::system::errc::make_error_code(boost::system::errc::address_in_use);
+ NDN_THROW_NO_STACK(fs::filesystem_error("UnixStreamChannel::listen", socketPath, ec));
+ }
+ else if (ec == boost::asio::error::connection_refused ||
+ ec == boost::asio::error::timed_out) {
+ // no one is listening on the remote side, we can safely remove the stale socket
+ NFD_LOG_CHAN_DEBUG("Removing stale socket file");
+ fs::remove(socketPath);
+ }
}
+ else if (type != fs::file_not_found) {
+ // the file exists but is not a socket: this is a fatal error as we cannot
+ // safely overwrite the file without potentially risking data loss
+ ec = boost::system::errc::make_error_code(boost::system::errc::not_a_socket);
+ NDN_THROW_NO_STACK(fs::filesystem_error("UnixStreamChannel::listen", socketPath, ec));
+ }
+
+ try {
+ m_acceptor.open();
+ m_acceptor.bind(m_endpoint);
+ m_acceptor.listen(backlog);
+ }
+ catch (const boost::system::system_error& e) {
+ // exceptions thrown by Boost.Asio are very terse, add more context
+ NDN_THROW_NO_STACK(fs::filesystem_error("UnixStreamChannel::listen: "s + e.std::runtime_error::what(),
+ socketPath, e.code()));
+ }
+
+ // do this here so that, even if the calls below fail,
+ // the destructor will still remove the socket file
+ m_isListening = true;
+
+ fs::permissions(socketPath, fs::owner_read | fs::group_read | fs::others_read |
+ fs::owner_write | fs::group_write | fs::others_write);
accept(onFaceCreated, onAcceptFailed);
NFD_LOG_CHAN_DEBUG("Started listening");
diff --git a/daemon/face/unix-stream-channel.hpp b/daemon/face/unix-stream-channel.hpp
index 7c7f287..dbe53d4 100644
--- a/daemon/face/unix-stream-channel.hpp
+++ b/daemon/face/unix-stream-channel.hpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2014-2023, Regents of the University of California,
+ * Copyright (c) 2014-2024, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -46,15 +46,6 @@
{
public:
/**
- * \brief UnixStreamChannel-related error.
- */
- class Error : public std::runtime_error
- {
- public:
- using std::runtime_error::runtime_error;
- };
-
- /**
* \brief Create a UnixStream channel for the specified \p endpoint.
*
* To enable the creation of faces upon incoming connections, one needs to
@@ -67,7 +58,7 @@
bool
isListening() const final
{
- return m_acceptor.is_open();
+ return m_isListening;
}
size_t
@@ -89,7 +80,7 @@
* returns an error)
* \param backlog The maximum length of the queue of pending incoming
* connections
- * \throw Error
+ * \throw boost::system::system_error
*/
void
listen(const FaceCreatedCallback& onFaceCreated,
@@ -104,6 +95,7 @@
private:
const unix_stream::Endpoint m_endpoint;
const bool m_wantCongestionMarking;
+ bool m_isListening = false;
boost::asio::local::stream_protocol::acceptor m_acceptor;
size_t m_size = 0;
};
diff --git a/daemon/main.cpp b/daemon/main.cpp
index bfaba73..725b7b6 100644
--- a/daemon/main.cpp
+++ b/daemon/main.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2014-2023, Regents of the University of California,
+ * Copyright (c) 2014-2024, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
@@ -36,10 +36,10 @@
#include <boost/asio/signal_set.hpp>
#include <boost/config.hpp>
#include <boost/exception/diagnostic_information.hpp>
-#include <boost/filesystem.hpp>
#include <boost/program_options/options_description.hpp>
#include <boost/program_options/parsers.hpp>
#include <boost/program_options/variables_map.hpp>
+#include <boost/system/system_error.hpp>
#include <boost/version.hpp>
#include <atomic>
@@ -317,13 +317,19 @@
", with ndn-cxx version " NDN_CXX_VERSION_BUILD_STRING
<< std::endl;
+ auto flush = ndn::make_scope_exit([] { ndn::util::Logging::flush(); });
+
NfdRunner runner(configFile);
try {
runner.initialize();
}
- catch (const boost::filesystem::filesystem_error& e) {
+ catch (const boost::system::system_error& e) {
NFD_LOG_FATAL(boost::diagnostic_information(e));
- return e.code() == boost::system::errc::permission_denied ? 4 : 1;
+ if (e.code() == boost::system::errc::operation_not_permitted ||
+ e.code() == boost::system::errc::permission_denied) {
+ return 4;
+ }
+ return 1;
}
catch (const std::exception& e) {
NFD_LOG_FATAL(boost::diagnostic_information(e));
diff --git a/tests/daemon/face/unix-stream-channel.t.cpp b/tests/daemon/face/unix-stream-channel.t.cpp
index d583625..a767d23 100644
--- a/tests/daemon/face/unix-stream-channel.t.cpp
+++ b/tests/daemon/face/unix-stream-channel.t.cpp
@@ -42,7 +42,17 @@
UnixStreamChannelFixture()
{
listenerEp = unix_stream::Endpoint(socketPath.string());
- fs::remove_all(socketPath.parent_path());
+
+ // in case an earlier test run crashed without a chance to run the destructor
+ boost::system::error_code ec;
+ fs::remove_all(testDir, ec);
+ }
+
+ ~UnixStreamChannelFixture() override
+ {
+ // cleanup
+ boost::system::error_code ec;
+ fs::remove_all(testDir, ec);
}
shared_ptr<UnixStreamChannel>
@@ -76,7 +86,8 @@
}
protected:
- fs::path socketPath = fs::path(UNIT_TESTS_TMPDIR) / "unix-stream-channel" / "test" / "foo.sock";
+ fs::path testDir = fs::path(UNIT_TESTS_TMPDIR) / "unix-stream-channel";
+ fs::path socketPath = testDir / "test" / "foo.sock";
};
BOOST_AUTO_TEST_SUITE(Face)
@@ -97,7 +108,7 @@
BOOST_CHECK_EQUAL(channel->isListening(), true);
// listen() is idempotent
- BOOST_CHECK_NO_THROW(channel->listen(nullptr, nullptr));
+ channel->listen(nullptr, nullptr);
BOOST_CHECK_EQUAL(channel->isListening(), true);
}
@@ -130,7 +141,9 @@
}
}
-BOOST_AUTO_TEST_CASE(SocketFile)
+BOOST_AUTO_TEST_SUITE(SocketFile)
+
+BOOST_AUTO_TEST_CASE(CreateAndRemove)
{
auto channel = makeChannel();
BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::file_not_found);
@@ -145,35 +158,128 @@
BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::file_not_found);
}
-BOOST_AUTO_TEST_CASE(ExistingSocketFile)
+BOOST_AUTO_TEST_CASE(InUse)
{
+ auto channel = makeChannel();
fs::create_directories(socketPath.parent_path());
+
local::stream_protocol::acceptor acceptor(g_io, listenerEp);
BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::socket_file);
- auto channel = makeChannel();
- BOOST_CHECK_THROW(channel->listen(nullptr, nullptr), UnixStreamChannel::Error);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::address_in_use &&
+ e.path1() == socketPath &&
+ std::string_view(e.what()).find("UnixStreamChannel::listen") != std::string_view::npos;
+ });
+}
+BOOST_AUTO_TEST_CASE(Stale)
+{
+ auto channel = makeChannel();
+ fs::create_directories(socketPath.parent_path());
+
+ local::stream_protocol::acceptor acceptor(g_io, listenerEp);
acceptor.close();
+ // the socket file is not removed when the acceptor is closed
BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::socket_file);
- BOOST_CHECK_NO_THROW(channel->listen(nullptr, nullptr));
+ // drop write permission from the parent directory
+ fs::permissions(socketPath.parent_path(), fs::owner_all & ~fs::owner_write);
+ // removal of the "stale" socket fails due to insufficient permissions
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::permission_denied &&
+ e.path1() == socketPath &&
+ std::string_view(e.what()).find("remove") != std::string_view::npos;
+ });
+ BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::socket_file);
+
+ // restore all permissions
+ fs::permissions(socketPath.parent_path(), fs::owner_all);
+ // the socket file should be considered "stale" and overwritten
+ channel->listen(nullptr, nullptr);
BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::socket_file);
}
-BOOST_AUTO_TEST_CASE(ExistingRegularFile)
+BOOST_AUTO_TEST_CASE(NotASocket)
{
- auto guard = ndn::make_scope_exit([=] { fs::remove(socketPath); });
+ auto channel = makeChannel();
- fs::create_directories(socketPath.parent_path());
+ fs::create_directories(socketPath);
+ BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::directory_file);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::not_a_socket &&
+ e.path1() == socketPath &&
+ std::string_view(e.what()).find("UnixStreamChannel::listen") != std::string_view::npos;
+ });
+
+ fs::remove(socketPath);
std::ofstream f(socketPath.string());
f.close();
BOOST_CHECK_EQUAL(fs::symlink_status(socketPath).type(), fs::regular_file);
-
- auto channel = makeChannel();
- BOOST_CHECK_THROW(channel->listen(nullptr, nullptr), UnixStreamChannel::Error);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::not_a_socket &&
+ e.path1() == socketPath &&
+ std::string_view(e.what()).find("UnixStreamChannel::listen") != std::string_view::npos;
+ });
}
+BOOST_AUTO_TEST_CASE(ParentConflict)
+{
+ auto channel = makeChannel();
+ fs::create_directories(testDir);
+
+ auto parent = socketPath.parent_path();
+ std::ofstream f(parent.string());
+ f.close();
+ BOOST_CHECK_EQUAL(fs::symlink_status(parent).type(), fs::regular_file);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::file_exists &&
+ e.path1() == parent &&
+ std::string_view(e.what()).find("create_dir") != std::string_view::npos;
+ });
+}
+
+BOOST_AUTO_TEST_CASE(PermissionDenied)
+{
+ auto channel = makeChannel();
+ fs::create_directories(testDir);
+
+ fs::permissions(testDir, fs::no_perms);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::permission_denied &&
+ e.path1() == socketPath.parent_path() &&
+ std::string_view(e.what()).find("create_dir") != std::string_view::npos;
+ });
+
+ fs::permissions(testDir, fs::owner_read | fs::owner_exe);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::permission_denied &&
+ e.path1() == socketPath.parent_path() &&
+ std::string_view(e.what()).find("create_dir") != std::string_view::npos;
+ });
+
+ fs::permissions(testDir, fs::owner_all);
+ fs::create_directories(socketPath.parent_path());
+
+ fs::permissions(socketPath.parent_path(), fs::no_perms);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::permission_denied &&
+ e.path1() == socketPath &&
+ std::string_view(e.what()).find("status") != std::string_view::npos;
+ });
+
+ fs::permissions(socketPath.parent_path(), fs::owner_read | fs::owner_exe);
+ BOOST_CHECK_EXCEPTION(channel->listen(nullptr, nullptr), fs::filesystem_error, [&] (const auto& e) {
+ return e.code() == boost::system::errc::permission_denied &&
+ e.path1() == socketPath &&
+ std::string_view(e.what()).find("bind") != std::string_view::npos;
+ });
+
+ fs::permissions(socketPath.parent_path(), fs::owner_all);
+}
+
+BOOST_AUTO_TEST_SUITE_END() // SocketFile
+
BOOST_AUTO_TEST_SUITE_END() // TestUnixStreamChannel
BOOST_AUTO_TEST_SUITE_END() // Face