diff --git a/common/io/Makefile.mk b/common/io/Makefile.mk index a19c5bafe2..8e480a677b 100644 --- a/common/io/Makefile.mk +++ b/common/io/Makefile.mk @@ -47,6 +47,7 @@ test_programs += \ common/io/MemoryBlockTester \ common/io/SelectServerTester \ common/io/StreamTester \ + common/io/SerialLockTester \ common/io/TimeoutManagerTester common_io_IOQueueTester_SOURCES = common/io/IOQueueTest.cpp @@ -78,3 +79,7 @@ common_io_StreamTester_SOURCES = common/io/InputStreamTest.cpp \ common/io/OutputStreamTest.cpp common_io_StreamTester_CXXFLAGS = $(COMMON_TESTING_FLAGS) common_io_StreamTester_LDADD = $(COMMON_TESTING_LIBS) + +common_io_SerialLockTester_SOURCES = common/io/SerialLockTester.cpp +common_io_SerialLockTester_CXXFLAGS = $(COMMON_TESTING_FLAGS) +common_io_SerialLockTester_LDADD = $(COMMON_TESTING_LIBS) diff --git a/common/io/Serial.cpp b/common/io/Serial.cpp index 9ffe6ca21b..334125e93a 100644 --- a/common/io/Serial.cpp +++ b/common/io/Serial.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #ifdef _WIN32 #define VC_EXTRALEAN #define WIN32_LEAN_AND_MEAN @@ -57,7 +58,7 @@ using std::string; namespace { -string GetLockFile(const string &path) { +string GetUUCPLockFile(const string &path) { const string base_name = ola::file::FilenameFromPath(path); return ola::file::JoinPaths(UUCP_LOCK_DIR, "LCK.." + base_name); } @@ -106,7 +107,7 @@ bool ProcessExists(pid_t pid) { #endif // _WIN32 } -bool RemoveLockFile(const string &lock_file) { +bool RemoveUUCPLockFile(const string &lock_file) { if (unlink(lock_file.c_str())) { OLA_WARN << "Failed to remove UUCP lock file: " << lock_file; return false; @@ -141,20 +142,12 @@ bool UIntToSpeedT(uint32_t value, speed_t *output) { } -bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) { +bool AcquireUUCPLock(const std::string &path) { // This is rather tricky since there is no real convention for LCK files. // If it was only a single process doing the locking we could use fnctl as // described in 55.6 of the Linux Programming Interface book. - // First, check if the path exists, there's no point trying to open it if not - if (!FileExists(path)) { - OLA_INFO << "Device " << path << " doesn't exist, so there's no point " - "trying to acquire a lock"; - return false; - } - - // Second, clean up a stale lockfile. - const string lock_file = GetLockFile(path); + const string lock_file = GetUUCPLockFile(path); OLA_DEBUG << "Checking for " << lock_file; pid_t locked_pid; if (!GetPidFromFile(lock_file, &locked_pid)) { @@ -162,6 +155,7 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) { return false; } + // Clean up a stale lockfile. if (locked_pid) { // This will return false even if we have the lock, this is what we want // since different plugins may try to open the same serial port - see issue @@ -173,7 +167,7 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) { } // There is a race between the read & the unlink here. I'm not convinced it // can be solved. - if (!RemoveLockFile(lock_file)) { + if (!RemoveUUCPLockFile(lock_file)) { OLA_INFO << "Device " << path << " was locked by PID " << locked_pid << " which is no longer active, however failed to remove stale " << "lock file"; @@ -207,7 +201,37 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) { close(lock_fd); if (r != pid_file_contents.size()) { OLA_WARN << "Failed to write complete LCK file: " << lock_file; - RemoveLockFile(lock_file); + RemoveUUCPLockFile(lock_file); + return false; + } + + return true; +} + +bool LockTIOCEXCL(int fd, const std::string &path) { +#if HAVE_SYS_IOCTL_H + // This is a final safety mechanism, on top of UUCP locking or flock() + if (ioctl(fd, TIOCEXCL) == -1) { + OLA_WARN << "TIOCEXCL " << path << " failed: " << strerror(errno); + return false; + } else { + OLA_INFO << "Set TIOCEXCL on " << path; + } +#else + OLA_INFO << "Not setting TIOCEXCL on " << path << " - ioctl.h unavailable"; +#endif // HAVE_SYS_IOCTL_H + return true; +} + +bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) { + // First, check if the path exists, there's no point trying to open it if not + if (!FileExists(path)) { + OLA_INFO << "Device " << path << " doesn't exist, so there's no point " + "trying to acquire a lock"; + return false; + } + + if (!AcquireUUCPLock(path)) { return false; } @@ -215,26 +239,25 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd) { if (!TryOpen(path, oflag, fd)) { OLA_DEBUG << "Failed to open device " << path << " despite having the " << "lock file"; - RemoveLockFile(lock_file); + close(*fd); + ReleaseUUCPLock(path); return false; } -#if HAVE_SYS_IOCTL_H // As a final safety mechanism, use ioctl(TIOCEXCL) if available to prevent // further opens. - if (ioctl(*fd, TIOCEXCL) == -1) { - OLA_WARN << "TIOCEXCL " << path << " failed: " << strerror(errno); + if (!LockTIOCEXCL(*fd, path)) { close(*fd); - RemoveLockFile(lock_file); + ReleaseUUCPLock(path); return false; } -#endif // HAVE_SYS_IOCTL_H + return true; } void ReleaseUUCPLock(const std::string &path) { - const string lock_file = GetLockFile(path); + const string lock_file = GetUUCPLockFile(path); pid_t locked_pid; if (!GetPidFromFile(lock_file, &locked_pid)) { @@ -243,10 +266,72 @@ void ReleaseUUCPLock(const std::string &path) { pid_t our_pid = getpid(); if (our_pid == locked_pid) { - if (RemoveLockFile(lock_file)) { + if (RemoveUUCPLockFile(lock_file)) { OLA_INFO << "Released " << lock_file; } } } + +bool AcquireLockAndOpenSerialPort(const std::string &path, int oflag, int *fd) { + // First, check if the path exists, there's no point trying to open it if not + if (!FileExists(path)) { + OLA_INFO << "Device " << path << " doesn't exist, so there's no point " + "trying to acquire a lock"; + return false; + } + +#ifdef UUCP_LOCKING + if (!AcquireUUCPLock(path)) { + return false; + } +#endif // UUCP_LOCKING + + // Now try to open the serial device. + if (!TryOpen(path, oflag, fd)) { +#ifdef UUCP_LOCKING + OLA_DEBUG << "Failed to open device " << path << " despite having the " + << "lock file"; + ReleaseUUCPLock(path); +#else + OLA_DEBUG << "Failed to open device " << path; +#endif // UUCP_LOCKING + return false; + } + +#ifdef HAVE_FLOCK + if (flock(*fd, LOCK_EX | LOCK_NB) == -1) { + OLA_INFO << "Failed to flock() device " << path; + close(*fd); +#ifdef UUCP_LOCKING + ReleaseUUCPLock(path); +#endif // UUCP_LOCKING + return false; + } else { + OLA_INFO << "Locked " << path << " using flock()"; + } +#endif // HAVE_FLOCK + + // As a final safety mechanism, use ioctl(TIOCEXCL) if available to prevent + // further opens. + if (!LockTIOCEXCL(*fd, path)) { + close(*fd); +#ifdef UUCP_LOCKING + ReleaseUUCPLock(path); +#endif // UUCP_LOCKING + return false; + } + + return true; +} + +void ReleaseSerialPortLock(const std::string &path) { +#ifdef UUCP_LOCKING + ReleaseUUCPLock(path); +#else + OLA_INFO << "No unlock necessary for " << path << " (UUCP locking not used)"; +#endif // UUCP_LOCKING +} + + } // namespace io } // namespace ola diff --git a/common/io/SerialLockTester.cpp b/common/io/SerialLockTester.cpp new file mode 100644 index 0000000000..54a4ff0f92 --- /dev/null +++ b/common/io/SerialLockTester.cpp @@ -0,0 +1,83 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * SerialLockTester.cpp + * Test for serial port locking + * Copyright (C) 2022 Thomas White + */ + +#if HAVE_CONFIG_H +#include +#endif // HAVE_CONFIG_H + +#include +#include +#include +#include +#include + +#include "ola/io/Serial.h" +#include "ola/io/IOUtils.h" +#include "ola/testing/TestUtils.h" + +class SerialLockTest: public CppUnit::TestFixture { + public: + CPPUNIT_TEST_SUITE(SerialLockTest); + CPPUNIT_TEST(testLock); + CPPUNIT_TEST_SUITE_END(); + + public: + void testLock(); +}; + + +CPPUNIT_TEST_SUITE_REGISTRATION(SerialLockTest); + +void SerialLockTest::testLock() { + int fd; + pid_t our_pid = getpid(); + + std::stringstream str; + str << "serialLockTestFile." << our_pid << ".pid"; + const std::string path = str.str(); + + OLA_ASSERT_FALSE_MSG(ola::io::FileExists(path), + "Test file already exists"); + + fd = open(path.c_str(), O_CREAT | O_RDWR, + S_IRUSR | S_IWUSR +#ifndef _WIN32 + | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH +#endif // !_WIN32 + ); // NOLINT(whitespace/parens) + OLA_ASSERT_FALSE_MSG(fd < 0, "Couldn't create test file"); + +#ifdef HAVE_FLOCK + OLA_ASSERT_TRUE(flock(fd, LOCK_EX | LOCK_NB) != -1); +#else + +#ifdef UUCP_LOCKING + // flock() is not available, but UUCP locking was selected. OK. +#else + OLA_FAIL("Not using UUCP locking, and flock() is not available"); +#endif // UUCP_LOCKING + +#endif // HAVE_FLOCK + + close(fd); + + OLA_ASSERT_FALSE_MSG(unlink(path.c_str()), + "Couldn't delete test file"); +} diff --git a/configure.ac b/configure.ac index f5046466b7..76feb1efc2 100644 --- a/configure.ac +++ b/configure.ac @@ -788,6 +788,23 @@ AC_ARG_ENABLE( [AS_HELP_STRING([--disable-doxygen-version], [Substitute the Doxygen version with latest, for the website])]) +# Use UUCP locking? +AC_ARG_ENABLE( + [uucp-locking], + [AS_HELP_STRING([--enable-uucp-locking], + [Use UUCP locking instead of flock()])]) + +AC_CHECK_FUNC([flock], [AC_DEFINE([HAVE_FLOCK], [1], + [Define if flock() is available]) + have_flock=yes], []) + +UUCP_LOCKING="no" +AS_IF([test "x$enable_uucp_locking" = xyes || test "x$have_flock" != xyes], + [AC_DEFINE([UUCP_LOCKING], [1], + [Define if UUCP locking should be used]) + UUCP_LOCKING=yes], + []) + # UUCP Lock directory AC_ARG_WITH([uucp-lock], [AS_HELP_STRING([--with-uucp-lock], @@ -1036,6 +1053,7 @@ Enable HTTP Server: ${have_microhttpd} RDM Responder Tests: ${enable_rdm_tests} Ja Rule: ${BUILDING_JA_RULE} Enabled Plugins:${PLUGINS} +UUCP Locking: $UUCP_LOCKING UUCP Lock Directory: $UUCPLOCK Now type 'make @<:@@:>@' diff --git a/include/ola/io/Serial.h b/include/ola/io/Serial.h index 17bbe15cd2..36be9e010f 100644 --- a/include/ola/io/Serial.h +++ b/include/ola/io/Serial.h @@ -67,6 +67,9 @@ bool UIntToSpeedT(uint32_t value, speed_t *output); * @returns true if the open succeeded, false otherwise. * * This fails-fast, it we can't get the lock immediately, we'll return false. + * + * @deprecated Use the more general AcquireLockAndOpenSerialPort() instead (19 Feb 2022). + * @see ReleaseUUCPLock() */ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd); @@ -75,8 +78,43 @@ bool AcquireUUCPLockAndOpen(const std::string &path, int oflag, int *fd); * @param path The path to unlock. * * The lock is only removed if the PID matches. + * + * @deprecated Use the more general ReleaseSerialPortLock() instead (19 Feb 2022). + * @see AcquireUUCPLockAndOpen() */ void ReleaseUUCPLock(const std::string &path); + +/** + * @brief Try to open the path and obtain a lock to control access + * @param path the path to open + * @param oflag flags passed to open + * @param[out] fd a pointer to the fd which is returned. + * @returns true if the open succeeded, false otherwise. + * + * Depending on the compile-time configuration, this will use as many as + * possible out of the flock() system call, UUCP lockfiles and the TIOCEXCL + * ioctl. UUCP lockfiles are disabled by default, unless flock() is + * unavailable. See: ./configure --enable-uucp-locking. + * + * This fails-fast, it we can't get the lock immediately, we'll return false. + * + * @see ReleaseSerialPortLock() + */ +bool AcquireLockAndOpenSerialPort(const std::string &path, int oflag, int *fd); + +/** + * @brief Release a lock for the serial port + * @param path The path to unlock. + * + * If UUCP locking was used (see AcquireLockAndOpenSerialPort()), and the PID + * in the lockfile matches our own, the lockfile will be removed. Otherwise, + * this call does nothing because the other locking methods do not require an + * explicit unlock. + * + * @see AcquireLockAndOpenSerialPort() + */ +void ReleaseSerialPortLock(const std::string &path); + } // namespace io } // namespace ola #endif // INCLUDE_OLA_IO_SERIAL_H_ diff --git a/plugins/stageprofi/StageProfiDetector.cpp b/plugins/stageprofi/StageProfiDetector.cpp index fab2908df9..3f0a035387 100644 --- a/plugins/stageprofi/StageProfiDetector.cpp +++ b/plugins/stageprofi/StageProfiDetector.cpp @@ -126,7 +126,7 @@ void StageProfiDetector::ReleaseWidget(const std::string &widget_path) { // the map. DescriptorMap::iterator iter = m_usb_widgets.find(widget_path); if (iter != m_usb_widgets.end()) { - ola::io::ReleaseUUCPLock(widget_path); + ola::io::ReleaseSerialPortLock(widget_path); iter->second = NULL; return; } @@ -164,9 +164,9 @@ ConnectedDescriptor* StageProfiDetector::ConnectToUSB( struct termios newtio; int fd; - if (!ola::io::AcquireUUCPLockAndOpen(widget_path, - O_RDWR | O_NONBLOCK | O_NOCTTY, - &fd)) { + if (!ola::io::AcquireLockAndOpenSerialPort(widget_path, + O_RDWR | O_NONBLOCK | O_NOCTTY, + &fd)) { return NULL; } diff --git a/plugins/usbpro/BaseUsbProWidget.cpp b/plugins/usbpro/BaseUsbProWidget.cpp index f92d2396eb..f36f4ec9a4 100644 --- a/plugins/usbpro/BaseUsbProWidget.cpp +++ b/plugins/usbpro/BaseUsbProWidget.cpp @@ -129,8 +129,9 @@ ola::io::ConnectedDescriptor *BaseUsbProWidget::OpenDevice( const string &path) { struct termios newtio; int fd; - if (!ola::io::AcquireUUCPLockAndOpen(path, O_RDWR | O_NONBLOCK | O_NOCTTY, - &fd)) { + if (!ola::io::AcquireLockAndOpenSerialPort(path, + O_RDWR | O_NONBLOCK | O_NOCTTY, + &fd)) { return NULL; } diff --git a/plugins/usbpro/WidgetDetectorThread.cpp b/plugins/usbpro/WidgetDetectorThread.cpp index 5f70ff52f0..692a3ee304 100644 --- a/plugins/usbpro/WidgetDetectorThread.cpp +++ b/plugins/usbpro/WidgetDetectorThread.cpp @@ -420,7 +420,7 @@ void WidgetDetectorThread::FreeDescriptor(ConnectedDescriptor *descriptor) { DescriptorInfo &descriptor_info = m_active_descriptors[descriptor]; m_active_paths.erase(descriptor_info.first); - io::ReleaseUUCPLock(descriptor_info.first); + io::ReleaseSerialPortLock(descriptor_info.first); m_active_descriptors.erase(descriptor); delete descriptor; }