From 4b1921fcf3cc8926151fd8e1932424d88573b855 Mon Sep 17 00:00:00 2001 From: Davide Pesavento Date: Fri, 12 Jan 2024 20:25:45 -0500 Subject: [PATCH] face: improve error handling in UnixStreamChannel Change-Id: I5d37b56e74264490089a4c18f527bbcc202a480d --- daemon/face/unix-stream-channel.cpp | 63 ++++++---- daemon/face/unix-stream-channel.hpp | 16 +-- daemon/main.cpp | 14 ++- tests/daemon/face/unix-stream-channel.t.cpp | 128 ++++++++++++++++++-- 4 files changed, 169 insertions(+), 52 deletions(-) diff --git a/daemon/face/unix-stream-channel.cpp b/daemon/face/unix-stream-channel.cpp index 150fad40..a1abe9c4 100644 --- a/daemon/face/unix-stream-channel.cpp +++ b/daemon/face/unix-stream-channel.cpp @@ -30,7 +30,6 @@ #include "common/global.hpp" #include -#include // for chmod() namespace nfd::face { @@ -50,10 +49,10 @@ UnixStreamChannel::~UnixStreamChannel() { 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 @@ UnixStreamChannel::listen(const FaceCreatedCallback& onFaceCreated, namespace fs = boost::filesystem; fs::path socketPath = m_endpoint.path(); - fs::file_type type = fs::symlink_status(socketPath).type(); + // ensure parent directory exists + fs::path parent = socketPath.parent_path(); + if (!parent.empty() && fs::create_directories(parent)) { + NFD_LOG_CHAN_TRACE("Created directory " << parent); + } + boost::system::error_code ec; + fs::file_type type = fs::symlink_status(socketPath).type(); if (type == fs::socket_file) { - boost::system::error_code error; + // 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, error); - NFD_LOG_CHAN_TRACE("connect() on existing socket file returned: " << error.message()); - if (!error) { + 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 - NDN_THROW(Error("Socket file at " + m_endpoint.path() + " belongs to another process")); + 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 (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 + 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) { - NDN_THROW(Error(m_endpoint.path() + " already exists and is not a socket file")); + // 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)); } - // ensure parent directory exists before creating socket - fs::path parent = socketPath.parent_path(); - if (!parent.empty() && fs::create_directories(parent)) { - NFD_LOG_CHAN_TRACE("Created directory " << parent); + 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())); } - m_acceptor.open(); - m_acceptor.bind(m_endpoint); - m_acceptor.listen(backlog); + // do this here so that, even if the calls below fail, + // the destructor will still remove the socket file + m_isListening = true; - if (::chmod(m_endpoint.path().data(), 0666) < 0) { - NDN_THROW_ERRNO(Error("Failed to chmod " + m_endpoint.path())); - } + 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 7c7f2873..dbe53d4f 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, @@ -45,15 +45,6 @@ namespace nfd::face { class UnixStreamChannel final : public Channel { 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. * @@ -67,7 +58,7 @@ class UnixStreamChannel final : public Channel bool isListening() const final { - return m_acceptor.is_open(); + return m_isListening; } size_t @@ -89,7 +80,7 @@ class UnixStreamChannel final : public Channel * 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 @@ class UnixStreamChannel final : public Channel 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 bfaba732..725b7b61 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 #include #include -#include #include #include #include +#include #include #include @@ -317,13 +317,19 @@ main(int argc, char** argv) ", 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 d5836251..a767d231 100644 --- a/tests/daemon/face/unix-stream-channel.t.cpp +++ b/tests/daemon/face/unix-stream-channel.t.cpp @@ -42,7 +42,17 @@ class UnixStreamChannelFixture : public ChannelFixture @@ -76,7 +86,8 @@ class UnixStreamChannelFixture : public ChannelFixtureisListening(), 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(MultipleAccepts) } } -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_AUTO_TEST_CASE(SocketFile) 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); + 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(); - BOOST_CHECK_THROW(channel->listen(nullptr, nullptr), UnixStreamChannel::Error); + 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); + + // 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); - BOOST_CHECK_NO_THROW(channel->listen(nullptr, nullptr)); + // 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); + 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(); - BOOST_CHECK_THROW(channel->listen(nullptr, nullptr), UnixStreamChannel::Error); + 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