From bc28790817931e4bd24cc651fec528e93a3a35b7 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
Date: Fri, 26 May 2023 23:48:09 -0700
Subject: [PATCH] Make sure hidapi error handling is thread-safe
The hidapi method of storing the error on the device is not thread-safe, and not only could it result in a double free if multiple threads were setting the error at the same time, but SDL could be trying to use the error message and have it be freed out from under it by another thread.
Use SDL's error functions since they already use thread-local storage.
---
src/hidapi/SDL_hidapi.c | 111 ++++----------------------------
src/hidapi/SDL_hidapi_libusb.h | 1 -
src/hidapi/SDL_hidapi_windows.h | 1 -
src/hidapi/linux/hid.c | 5 ++
src/hidapi/mac/hid.c | 5 ++
src/hidapi/windows/hid.c | 24 ++++++-
6 files changed, 46 insertions(+), 101 deletions(-)
diff --git a/src/hidapi/SDL_hidapi.c b/src/hidapi/SDL_hidapi.c
index 6ca699c64251..2dda506b8963 100644
--- a/src/hidapi/SDL_hidapi.c
+++ b/src/hidapi/SDL_hidapi.c
@@ -528,7 +528,8 @@ static void HIDAPI_ShutdownDiscovery(void)
/* Platform HIDAPI Implementation */
-#define HIDAPI_IGNORE_DEVICE(VID, PID) SDL_HIDAPI_ShouldIgnoreDevice(VID, PID)
+#define HIDAPI_USING_SDL_RUNTIME
+#define HIDAPI_IGNORE_DEVICE(VID, PID) SDL_HIDAPI_ShouldIgnoreDevice(VID, PID)
struct PLATFORM_hid_device_;
typedef struct PLATFORM_hid_device_ PLATFORM_hid_device;
@@ -1034,17 +1035,6 @@ static void CopyHIDDeviceInfo(struct hid_device_info *pSrc, struct SDL_hid_devic
static int SDL_hidapi_refcount = 0;
static char *SDL_hidapi_ignored_devices = NULL;
-static void SDL_SetHIDAPIError(const wchar_t *error)
-{
- if (error) {
- char *error_utf8 = SDL_iconv_wchar_utf8(error);
- if (error_utf8) {
- SDL_SetError("%s", error_utf8);
- SDL_free(error_utf8);
- }
- }
-}
-
static void SDLCALL IgnoredDevicesChanged(void *userdata, const char *name, const char *oldValue, const char *hint)
{
if (SDL_hidapi_ignored_devices) {
@@ -1491,93 +1481,51 @@ SDL_hid_device *SDL_hid_open_path(const char *path)
int SDL_hid_write(SDL_hid_device *device, const unsigned char *data, size_t length)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_write(device->device, data, length);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_write(device->device, data, length);
}
int SDL_hid_read_timeout(SDL_hid_device *device, unsigned char *data, size_t length, int milliseconds)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_read_timeout(device->device, data, length, milliseconds);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_read_timeout(device->device, data, length, milliseconds);
}
int SDL_hid_read(SDL_hid_device *device, unsigned char *data, size_t length)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_read(device->device, data, length);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_read(device->device, data, length);
}
int SDL_hid_set_nonblocking(SDL_hid_device *device, int nonblock)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_set_nonblocking(device->device, nonblock);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_set_nonblocking(device->device, nonblock);
}
int SDL_hid_send_feature_report(SDL_hid_device *device, const unsigned char *data, size_t length)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_send_feature_report(device->device, data, length);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_send_feature_report(device->device, data, length);
}
int SDL_hid_get_feature_report(SDL_hid_device *device, unsigned char *data, size_t length)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_feature_report(device->device, data, length);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_feature_report(device->device, data, length);
}
int SDL_hid_get_input_report(SDL_hid_device *device, unsigned char *data, size_t length)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_input_report(device->device, data, length);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_input_report(device->device, data, length);
}
int SDL_hid_close(SDL_hid_device *device)
@@ -1591,54 +1539,30 @@ int SDL_hid_close(SDL_hid_device *device)
int SDL_hid_get_manufacturer_string(SDL_hid_device *device, wchar_t *string, size_t maxlen)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_manufacturer_string(device->device, string, maxlen);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_manufacturer_string(device->device, string, maxlen);
}
int SDL_hid_get_product_string(SDL_hid_device *device, wchar_t *string, size_t maxlen)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_product_string(device->device, string, maxlen);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_product_string(device->device, string, maxlen);
}
int SDL_hid_get_serial_number_string(SDL_hid_device *device, wchar_t *string, size_t maxlen)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_serial_number_string(device->device, string, maxlen);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_serial_number_string(device->device, string, maxlen);
}
int SDL_hid_get_indexed_string(SDL_hid_device *device, int string_index, wchar_t *string, size_t maxlen)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_indexed_string(device->device, string_index, string, maxlen);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_indexed_string(device->device, string_index, string, maxlen);
}
SDL_hid_device_info *SDL_hid_get_device_info(SDL_hid_device *device)
@@ -1652,22 +1576,15 @@ SDL_hid_device_info *SDL_hid_get_device_info(SDL_hid_device *device)
CopyHIDDeviceInfo(info, &device->info);
return &device->info;
} else {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
return NULL;
}
}
int SDL_hid_get_report_descriptor(SDL_hid_device *device, unsigned char *buf, size_t buf_size)
{
- int result;
-
CHECK_DEVICE_MAGIC(device, -1);
- result = device->backend->hid_get_report_descriptor(device->device, buf, buf_size);
- if (result < 0) {
- SDL_SetHIDAPIError(device->backend->hid_error(device->device));
- }
- return result;
+ return device->backend->hid_get_report_descriptor(device->device, buf, buf_size);
}
void SDL_hid_ble_scan(SDL_bool active)
diff --git a/src/hidapi/SDL_hidapi_libusb.h b/src/hidapi/SDL_hidapi_libusb.h
index e0a124833834..c3dc83d4cf3a 100644
--- a/src/hidapi/SDL_hidapi_libusb.h
+++ b/src/hidapi/SDL_hidapi_libusb.h
@@ -20,7 +20,6 @@
*/
/* Define standard library functions in terms of SDL */
-#define HIDAPI_USING_SDL_RUNTIME
#define free SDL_free
#define iconv_t SDL_iconv_t
#define ICONV_CONST
diff --git a/src/hidapi/SDL_hidapi_windows.h b/src/hidapi/SDL_hidapi_windows.h
index e0fbd473e4d5..de1c5f70ab2b 100644
--- a/src/hidapi/SDL_hidapi_windows.h
+++ b/src/hidapi/SDL_hidapi_windows.h
@@ -20,7 +20,6 @@
*/
/* Define standard library functions in terms of SDL */
-#define HIDAPI_USING_SDL_RUNTIME
#define calloc SDL_calloc
#define free SDL_free
#define malloc SDL_malloc
diff --git a/src/hidapi/linux/hid.c b/src/hidapi/linux/hid.c
index 9498a85569aa..7274aeff96b7 100644
--- a/src/hidapi/linux/hid.c
+++ b/src/hidapi/linux/hid.c
@@ -131,7 +131,12 @@ static wchar_t *utf8_to_wchar_t(const char *utf8)
static void register_error_str(wchar_t **error_str, const char *msg)
{
free(*error_str);
+#ifdef HIDAPI_USING_SDL_RUNTIME
+ /* Thread-safe error handling */
+ SDL_SetError("%s", msg);
+#else
*error_str = utf8_to_wchar_t(msg);
+#endif
}
/* Semilar to register_error_str, but allows passing a format string with va_list args into this function. */
diff --git a/src/hidapi/mac/hid.c b/src/hidapi/mac/hid.c
index 9bc4d52cfcf6..20e898e7e0b5 100644
--- a/src/hidapi/mac/hid.c
+++ b/src/hidapi/mac/hid.c
@@ -244,7 +244,12 @@ static wchar_t *utf8_to_wchar_t(const char *utf8)
static void register_error_str(wchar_t **error_str, const char *msg)
{
free(*error_str);
+#ifdef HIDAPI_USING_SDL_RUNTIME
+ /* Thread-safe error handling */
+ SDL_SetError("%s", msg);
+#else
*error_str = utf8_to_wchar_t(msg);
+#endif
}
/* Similar to register_error_str, but allows passing a format string with va_list args into this function. */
diff --git a/src/hidapi/windows/hid.c b/src/hidapi/windows/hid.c
index b7389d712007..992affb71283 100644
--- a/src/hidapi/windows/hid.c
+++ b/src/hidapi/windows/hid.c
@@ -285,8 +285,7 @@ static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR
+ system_err_len
;
- *error_buffer = (WCHAR *)calloc(msg_len + 1, sizeof (WCHAR));
- WCHAR *msg = *error_buffer;
+ WCHAR *msg = (WCHAR *)calloc(msg_len + 1, sizeof (WCHAR));
if (!msg)
return;
@@ -307,6 +306,18 @@ static void register_winapi_error_to_buffer(wchar_t **error_buffer, const WCHAR
msg[msg_len-1] = L'\0';
msg_len--;
}
+
+#ifdef HIDAPI_USING_SDL_RUNTIME
+ /* Thread-safe error handling */
+ char *error_utf8 = SDL_iconv_wchar_utf8(msg);
+ if (error_utf8) {
+ SDL_SetError("%s", error_utf8);
+ SDL_free(error_utf8);
+ }
+ free(msg);
+#else
+ *error_buffer = msg;
+#endif
}
#if defined(__GNUC__)
@@ -324,7 +335,16 @@ static void register_string_error_to_buffer(wchar_t **error_buffer, const WCHAR
*error_buffer = NULL;
if (string_error) {
+#ifdef HIDAPI_USING_SDL_RUNTIME
+ /* Thread-safe error handling */
+ char *error_utf8 = SDL_iconv_wchar_utf8(string_error);
+ if (error_utf8) {
+ SDL_SetError("%s", error_utf8);
+ SDL_free(error_utf8);
+ }
+#else
*error_buffer = _wcsdup(string_error);
+#endif
}
}