From bc28790817931e4bd24cc651fec528e93a3a35b7 Mon Sep 17 00:00:00 2001 From: Sam Lantinga 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 6ca699c642..2dda506b89 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 e0a1248338..c3dc83d4cf 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 e0fbd473e4..de1c5f70ab 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 9498a85569..7274aeff96 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 9bc4d52cfc..20e898e7e0 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 b7389d7120..992affb712 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 } }