Fix signal handling and add atomic operations for thread safety

hid_read_test/CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.8...3.25 FATAL_ERROR)
+cmake_minimum_required(VERSION 3.1.3...3.25 FATAL_ERROR)
 project(hid_read_test CXX)
 
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
@@ -19,20 +19,32 @@ set(HIDAPI_HID_READ_TEST_TARGETS)
 if(NOT WIN32 AND NOT APPLE AND CMAKE_SYSTEM_NAME MATCHES "Linux")
     if(TARGET hidapi::hidraw)
         add_executable(hid_read_test_hidraw main.cpp)
-        target_compile_features(hid_read_test_hidraw PRIVATE cxx_std_11)
+        set_target_properties(hid_read_test_hidraw PROPERTIES
+            CXX_STANDARD 11
+            CXX_STANDARD_REQUIRED ON
+            CXX_EXTENSIONS OFF
+        )
         target_link_libraries(hid_read_test_hidraw PRIVATE hidapi::hidraw Threads::Threads)
         list(APPEND HIDAPI_HID_READ_TEST_TARGETS hid_read_test_hidraw)
     endif()
     if(TARGET hidapi::libusb)
         add_executable(hid_read_test_libusb main.cpp)
-        target_compile_features(hid_read_test_libusb PRIVATE cxx_std_11)
+        set_target_properties(hid_read_test_libusb PROPERTIES
+            CXX_STANDARD 11
+            CXX_STANDARD_REQUIRED ON
+            CXX_EXTENSIONS OFF
+        )
         target_compile_definitions(hid_read_test_libusb PRIVATE USING_HIDAPI_LIBUSB)
         target_link_libraries(hid_read_test_libusb PRIVATE hidapi::libusb Threads::Threads)
         list(APPEND HIDAPI_HID_READ_TEST_TARGETS hid_read_test_libusb)
     endif()
 else()
     add_executable(hid_read_test main.cpp)
-    target_compile_features(hid_read_test PRIVATE cxx_std_11)
+    set_target_properties(hid_read_test PROPERTIES
+        CXX_STANDARD 11
+        CXX_STANDARD_REQUIRED ON
+        CXX_EXTENSIONS OFF
+    )
     target_link_libraries(hid_read_test PRIVATE hidapi::hidapi Threads::Threads)
     list(APPEND HIDAPI_HID_READ_TEST_TARGETS hid_read_test)
 endif()

hid_read_test/main.cpp

@@ -24,9 +24,13 @@
 #include <string>
 #include <thread>
 
+#ifndef _WIN32
+#include <signal.h>
+#endif
+
 namespace {
 
-std::atomic<hid_device*> g_dev{nullptr};
+std::atomic<bool> g_terminate{false};
 
 std::string timestamp_now()
 {
@@ -50,8 +54,9 @@ std::string timestamp_now()
 
 extern "C" void on_signal(int)
 {
-    if (hid_device *d = g_dev.load(std::memory_order_acquire))
-        hid_read_interrupt(d);
+    /* async-signal-safe: atomic store only. Main thread will call
+       hid_read_interrupt() once cin.get() returns from EINTR. */
+    g_terminate.store(true, std::memory_order_release);
 }
 
 void read_thread_fn(hid_device *dev)
@@ -122,12 +127,20 @@ int main(int argc, char **argv)
         hid_exit();
         return 1;
     }
-    g_dev.store(dev, std::memory_order_release);
-
+#ifdef _WIN32
     std::signal(SIGINT, on_signal);
 #ifdef SIGTERM
     std::signal(SIGTERM, on_signal);
 #endif
+#else
+    /* Use sigaction without SA_RESTART so cin.get() returns on signal. */
+    struct sigaction sa;
+    sa.sa_handler = on_signal;
+    sigemptyset(&sa.sa_mask);
+    sa.sa_flags = 0;
+    sigaction(SIGINT, &sa, nullptr);
+    sigaction(SIGTERM, &sa, nullptr);
+#endif
 
     std::cout << "Reading from VID=" << std::hex << std::setw(4) << std::setfill('0')
               << vid << " PID=" << std::setw(4) << pid << std::dec

linux/hid.c

@@ -80,7 +80,7 @@ struct hid_device_ {
 	wchar_t *last_read_error_str;
 	struct hid_device_info* device_info;
 	int interrupt_efd;
-	volatile int interrupted;
+	int interrupted;
 };
 
 static struct hid_api_version api_version = {
@@ -107,7 +107,9 @@ static hid_device *new_hid_device(void)
 	dev->interrupted = 0;
 	dev->interrupt_efd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
 	if (dev->interrupt_efd < 0) {
+		int saved_errno = errno;
 		free(dev);
+		errno = saved_errno;
 		return NULL;
 	}
 
@@ -1091,8 +1093,7 @@ hid_device * HID_API_EXPORT hid_open_path(const char *path)
 
 	dev = new_hid_device();
 	if (!dev) {
-		errno = ENOMEM;
-		register_global_error("Couldn't allocate memory");
+		register_global_error_format("Failed to create hid_device: %s", strerror(errno));
 		return NULL;
 	}
 
@@ -1149,7 +1150,7 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t
 	/* Set device error to none */
 	register_error_str(&dev->last_read_error_str, NULL);
 
-	if (dev->interrupted) {
+	if (__atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE)) {
 		register_error_str(&dev->last_read_error_str, "hid_read_timeout: operation interrupted");
 		return -1;
 	}
@@ -1238,7 +1239,7 @@ int HID_API_EXPORT hid_set_nonblocking(hid_device *dev, int nonblock)
 
 int HID_API_EXPORT hid_read_interrupt(hid_device *dev)
 {
-	dev->interrupted = 1;
+	__atomic_store_n(&dev->interrupted, 1, __ATOMIC_RELEASE);
 	uint64_t one = 1;
 	ssize_t written = write(dev->interrupt_efd, &one, sizeof(one));
 	(void)written;
@@ -1247,12 +1248,12 @@ int HID_API_EXPORT hid_read_interrupt(hid_device *dev)
 
 int HID_API_EXPORT hid_is_read_interrupted(hid_device *dev)
 {
-	return dev->interrupted;
+	return __atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE);
 }
 
 int HID_API_EXPORT hid_read_clear_interrupt(hid_device *dev)
 {
-	dev->interrupted = 0;
+	__atomic_store_n(&dev->interrupted, 0, __ATOMIC_RELEASE);
 	uint64_t v;
 	ssize_t got = read(dev->interrupt_efd, &v, sizeof(v));
 	(void)got;
@@ -1341,7 +1342,8 @@ void HID_API_EXPORT hid_close(hid_device *dev)
 	if (!dev)
 		return;
 
-	close(dev->device_handle);
+	if (dev->device_handle >= 0)
+		close(dev->device_handle);
 	if (dev->interrupt_efd >= 0)
 		close(dev->interrupt_efd);
 

netbsd/hid.c

@@ -60,7 +60,7 @@ struct hid_device_ {
 	int report_handles[256];
 	char path[USB_MAX_DEVNAMELEN];
 	int interrupt_pipe[2];
-	volatile int interrupted;
+	int interrupted;
 };
 
 struct hid_enumerate_data {
@@ -932,7 +932,7 @@ int HID_API_EXPORT HID_API_CALL hid_read_timeout(hid_device *dev, unsigned char
 
 	register_device_read_error(dev, NULL);
 
-	if (dev->interrupted) {
+	if (__atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE)) {
 		register_device_read_error(dev, "hid_read_timeout: operation interrupted");
 		return -1;
 	}
@@ -995,7 +995,7 @@ int HID_API_EXPORT HID_API_CALL hid_set_nonblocking(hid_device *dev, int nonbloc
 
 int HID_API_EXPORT HID_API_CALL hid_read_interrupt(hid_device *dev)
 {
-	dev->interrupted = 1;
+	__atomic_store_n(&dev->interrupted, 1, __ATOMIC_RELEASE);
 	const char one = 1;
 	ssize_t written = write(dev->interrupt_pipe[1], &one, 1);
 	(void)written;
@@ -1004,12 +1004,12 @@ int HID_API_EXPORT HID_API_CALL hid_read_interrupt(hid_device *dev)
 
 int HID_API_EXPORT HID_API_CALL hid_is_read_interrupted(hid_device *dev)
 {
-	return dev->interrupted;
+	return __atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE);
 }
 
 int HID_API_EXPORT HID_API_CALL hid_read_clear_interrupt(hid_device *dev)
 {
-	dev->interrupted = 0;
+	__atomic_store_n(&dev->interrupted, 0, __ATOMIC_RELEASE);
 	char buf[64];
 	ssize_t got;
 	do {

windows/hid.c

@@ -1283,9 +1283,9 @@ int HID_API_EXPORT HID_API_CALL hid_read_interrupt(hid_device *dev)
 
 int HID_API_EXPORT HID_API_CALL hid_is_read_interrupted(hid_device *dev)
 {
-	// TODO: very common but not the most efficient way to check this,
+	// TODO: common but not the most efficient way to check this;
 	// move to C11 atomics when the time comes.
-	// For now this is the most portable and efficient enought.
+	// For now this is portable and efficient enough.
 	return InterlockedCompareExchange(&dev->interrupted, 0, 0) ? 1 : 0;
 }