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

@@ -528,6 +528,7 @@ static void HIDAPI_ShutdownDiscovery(void)
 
 /* Platform HIDAPI Implementation */
 
+#define HIDAPI_USING_SDL_RUNTIME
 #define HIDAPI_IGNORE_DEVICE(VID, PID)      SDL_HIDAPI_ShouldIgnoreDevice(VID, PID)
 
 struct 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);
+    return device->backend->hid_write(device->device, data, length);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_read_timeout(device->device, data, length, milliseconds);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_read(device->device, data, length);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_set_nonblocking(device->device, nonblock);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_send_feature_report(device->device, data, length);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_get_feature_report(device->device, data, length);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_get_input_report(device->device, data, length);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_get_manufacturer_string(device->device, string, maxlen);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_get_product_string(device->device, string, maxlen);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return device->backend->hid_get_serial_number_string(device->device, string, maxlen);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 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);
+    return 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;
 }
 
 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);
+    return device->backend->hid_get_report_descriptor(device->device, buf, buf_size);
-    if (result < 0) {
-        SDL_SetHIDAPIError(device->backend->hid_error(device->device));
-    }
-    return result;
 }
 
 void SDL_hid_ble_scan(SDL_bool active)

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

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

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. */

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. */

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 = (WCHAR *)calloc(msg_len + 1, sizeof (WCHAR));
-	WCHAR *msg = *error_buffer;
 
 	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
 	}
 }