Use flock() instead of UUCP-style locking for serial devices

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)

common/io/Serial.cpp

@@ -27,6 +27,7 @@
 #include <signal.h>
 #include <string.h>
 #include <sys/stat.h>
+#include <sys/file.h>
 #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,27 +142,20 @@ 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)) {
     OLA_INFO << "Failed to get PID from  " << lock_file;
     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,34 +201,63 @@ 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;
   }
 
   // Now try to open the serial device.
   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

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 <config.h>
+#endif  // HAVE_CONFIG_H
+
+#include <sys/file.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <cppunit/extensions/HelperMacros.h>
+#include <string>
+
+#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");
+}

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 @<:@<target>@:>@'

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_