From: Francis Dupont Date: Sun, 14 Sep 2025 09:12:29 +0000 (+0200) Subject: [#4107] Added PIDLock RAII X-Git-Tag: Kea-3.1.2~32 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be8df7a17555c3e73967f31fcb9ea446eb81368a;p=thirdparty%2Fkea.git [#4107] Added PIDLock RAII --- diff --git a/src/bin/lfc/lfc_controller.cc b/src/bin/lfc/lfc_controller.cc index 9da33bc3b7..cd651d305b 100644 --- a/src/bin/lfc/lfc_controller.cc +++ b/src/bin/lfc/lfc_controller.cc @@ -83,7 +83,10 @@ LFCController::launch(int argc, char* argv[], const bool test_mode) { PIDFile pid_file(pid_file_); try { - if (pid_file.check()) { + // Acquire a lock for check and write operations. + PIDLock pid_lock(pid_file.getLockname()); + + if (!pid_lock.isLocked() || pid_file.check()) { // Already running instance, bail out LOG_FATAL(lfc_logger, LFC_RUNNING); return; diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index f858b8c788..00371308da 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -2267,7 +2267,8 @@ Memfile_LeaseMgr::isLFCProcessRunning(const std::string file_name, Universe u) { lease_file = Memfile_LeaseMgr::getDefaultLeaseFilePath(u); } PIDFile pid_file(Memfile_LeaseMgr::appendSuffix(lease_file, FILE_PID)); - return (pid_file.check()); + PIDLock pid_lock(pid_file.getLockname()); + return (!pid_lock.isLocked() || pid_file.check()); } std::string diff --git a/src/lib/process/daemon.cc b/src/lib/process/daemon.cc index cba9ded080..d95e38651b 100644 --- a/src/lib/process/daemon.cc +++ b/src/lib/process/daemon.cc @@ -176,6 +176,15 @@ Daemon::getPIDFileName() const { return (""); } +std::string +Daemon::getPIDLockName() const { + if (pid_file_) { + return (pid_file_->getLockname()); + } + + return (""); +} + void Daemon::setPIDFileName(const std::string& pid_file_name) { if (pid_file_) { @@ -228,6 +237,13 @@ Daemon::createPIDFile(int pid) { setPIDFileName(makePIDFileName()); } + // Acquire a lock for check and write operations. + util::PIDLock pid_lock(getPIDLockName()); + if (!pid_lock.isLocked()) { + isc_throw(DaemonPIDExists, "Daemon::createPIDFile: can't lock, " + "PID lock file: " << getPIDLockName()); + } + // If we find a pre-existing file containing a live PID we bail. int chk_pid = pid_file_->check(); if (chk_pid > 0) { diff --git a/src/lib/process/daemon.h b/src/lib/process/daemon.h index 791d811f71..1271fc85e1 100644 --- a/src/lib/process/daemon.h +++ b/src/lib/process/daemon.h @@ -189,6 +189,10 @@ public: /// @return text string std::string getPIDFileName() const; + /// @brief Returns the current PID lock file name + /// @return text string + std::string getPIDLockName() const; + /// @brief Sets PID file name /// /// If this method is called prior to calling createPIDFile, @@ -202,11 +206,13 @@ public: /// @brief Creates the PID file /// /// If the PID file name has not been previously set, the method - /// uses manufacturePIDFileName() to set it. If the PID file + /// uses makePIDFileName() to set it. If the PID file /// name refers to an existing file whose contents are a PID whose /// process is still alive, the method will throw a DaemonPIDExists /// exception. Otherwise, the file created (or truncated) and /// the given pid (if not zero) is written to the file. + /// As there is a race condition between the check and write phases + /// a lock is taken. /// /// @param pid PID to write to the file if not zero, otherwise the /// PID of the current process is used. diff --git a/src/lib/util/pid_file.cc b/src/lib/util/pid_file.cc index 89d2681ebc..c18f9e6b9f 100644 --- a/src/lib/util/pid_file.cc +++ b/src/lib/util/pid_file.cc @@ -11,17 +11,11 @@ #include #include #include +#include namespace isc { namespace util { -PIDFile::PIDFile(const std::string& filename) - : filename_(filename) { -} - -PIDFile::~PIDFile() { -} - int PIDFile::check() const { std::ifstream fs(filename_.c_str()); @@ -89,5 +83,45 @@ PIDFile::deleteFile() const { } } +PIDLock::PIDLock(const std::string& lockname) + : lockname_(lockname), fd_(-1), locked_(false) { + // Open the lock file. + fd_ = open(lockname_.c_str(), O_RDONLY | O_CREAT, 0600); + if (fd_ == -1) { + if (errno == ENOENT) { + // Ignoring missing component in the path. + locked_ = true; + return; + } + std::string errmsg = strerror(errno); + isc_throw(PIDFileError, "cannot create pid lockfile '" + << lockname_ << "': " << errmsg); + } + // Try to acquire the lock. If we can't somebody else is actively + // using it. + int ret = flock(fd_, LOCK_EX | LOCK_NB); + if (ret == 0) { + locked_ = true; + return; + } + if (errno != EWOULDBLOCK) { + std::string errmsg = strerror(errno); + isc_throw(PIDFileError, "cannot lock pid lockfile '" + << lockname_ << "': " << errmsg); + } +} + +PIDLock::~PIDLock() { + if (fd_ != -1) { + if (locked_) { + // For symmetry as the close releases the lock... + static_cast(flock(fd_, LOCK_UN)); + } + static_cast(close(fd_)); + } + fd_ = -1; + locked_ = false; +} + } // namespace isc::util } // namespace isc diff --git a/src/lib/util/pid_file.h b/src/lib/util/pid_file.h index 43d72b9ce0..6637e2cdcc 100644 --- a/src/lib/util/pid_file.h +++ b/src/lib/util/pid_file.h @@ -42,10 +42,12 @@ public: /// @brief Constructor /// /// @param filename PID filename. - PIDFile(const std::string& filename); + PIDFile(const std::string& filename) + : filename_(filename) { + } /// @brief Destructor - ~PIDFile(); + virtual ~PIDFile() = default; /// @brief Read the PID in from the file and check it. /// @@ -55,6 +57,8 @@ public: /// If the file exists but can't be read or it doesn't have /// the proper format treat it as the process existing. /// + /// The PID file should be locked to avoid a race condition. + /// /// @return returns the PID if it is in, 0 otherwise. /// /// @throw throws PIDCantReadPID if it was able to open the file @@ -63,6 +67,8 @@ public: /// @brief Write the PID to the file. /// + /// The PID file must be locked to avoid a race condition. + /// /// @param pid the pid to write to the file. /// /// @throw throws PIDFileError if it can't open or write to the PID file. @@ -70,11 +76,15 @@ public: /// @brief Get PID of the current process and write it to the file. /// + /// The PID file must be locked to avoid a race condition. + /// /// @throw throws PIDFileError if it can't open or write to the PID file. void write() const; /// @brief Delete the PID file. /// + /// This is an atomic operation not subject to a race condition. + /// /// @throw throws PIDFileError if it can't delete the PID file void deleteFile() const; @@ -83,6 +93,11 @@ public: return (filename_); } + /// @brief Returns the path to the lock file. + std::string getLockname() const { + return (filename_ + ".lock"); + } + private: /// @brief PID filename std::string filename_; @@ -91,6 +106,37 @@ private: /// @brief Defines a shared pointer to a PIDFile typedef boost::shared_ptr PIDFilePtr; +/// @brief RAII device to handle a lock file to avoid race conditions. +class PIDLock { +public: + /// @brief Constructor + /// + /// Try to get a lock. + /// + /// @param lockname Lock filename. + PIDLock(const std::string& lockname); + + /// @brief Destructor + /// + /// Release the lock when taken and delete the lock file. + ~PIDLock(); + + /// @brief Return the lock status. + bool isLocked() { + return (locked_); + } + +private: + /// @brief Name of the Lock file. + std::string lockname_; + + /// @brief File descriptor to the lock file. + int fd_; + + /// @brief Lock status. + bool locked_; +}; + } // namespace isc::util } // namespace isc diff --git a/src/lib/util/tests/pid_file_unittest.cc b/src/lib/util/tests/pid_file_unittest.cc index 5f00d724e3..ac1cd71301 100644 --- a/src/lib/util/tests/pid_file_unittest.cc +++ b/src/lib/util/tests/pid_file_unittest.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -17,6 +18,7 @@ using namespace isc::util; // Filenames used for testing. const char* TESTNAME = "pid_file.test"; +const char* TESTLOCKNAME = "pid_file.test.lock"; class PIDFileTest : public ::testing::Test { public: @@ -54,18 +56,19 @@ public: protected: /// @brief Removes any old test files before the test virtual void SetUp() { - removeTestFile(); + removeTestFiles(); } /// @brief Removes any remaining test files after the test virtual void TearDown() { - removeTestFile(); + removeTestFiles(); } private: /// @brief Removes any remaining test files - void removeTestFile() const { + void removeTestFiles() const { static_cast(remove(absolutePath(TESTNAME).c_str())); + static_cast(remove(absolutePath(TESTLOCKNAME).c_str())); } }; @@ -203,4 +206,37 @@ TEST_F(PIDFileTest, noDeleteFile) { // Delete a file we haven't created pid_file.deleteFile(); } + +/// @brief Test getting a lock. +TEST_F(PIDFileTest, lock) { + PIDFile pid_file(absolutePath(TESTNAME)); + EXPECT_EQ(absolutePath(TESTLOCKNAME), pid_file.getLockname()); + + PIDLock lock(absolutePath(TESTLOCKNAME)); + EXPECT_TRUE(lock.isLocked()); + + PIDLock lock2(absolutePath(TESTLOCKNAME)); + EXPECT_FALSE(lock2.isLocked()); +} + +/// @brief Test getting and releasing a lock. +TEST_F(PIDFileTest, lock2) { + { + PIDLock lock(absolutePath(TESTLOCKNAME)); + EXPECT_TRUE(lock.isLocked()); + } + { + PIDLock lock2(absolutePath(TESTLOCKNAME)); + EXPECT_TRUE(lock2.isLocked()); + } +} + +/// @brief Test ignoring a path with a missing component. +TEST_F(PIDFileTest, lockNoent) { + boost::scoped_ptr lock; + ASSERT_NO_THROW(lock.reset(new PIDLock("/does/not/exist.lock"))); + ASSERT_TRUE(lock); + EXPECT_TRUE(lock->isLocked()); +} + } // end of anonymous namespace