Add a bunch of sanity checks #696

Megamouse · 2024-09-25T20:31:37Z

Some of these changes are meant to fix a couple of warnings while reading the code.
I also noticed that there were almost no sanity checks in the code, which may lead to lots of segfaults in user code.

Win: Do not call CloseHandle in hid_open_path if device_handle is null
Win: return errors if malloc calls fail
Add missing NULL checks to many parameters

libusb/hid.c

linux/hid.c

Youw · 2024-09-26T08:58:02Z

windows/hid.c

@@ -318,11 +324,17 @@ static void register_string_error_to_buffer(wchar_t **error_buffer, const WCHAR

 static void register_winapi_error(hid_device *dev, const WCHAR *op)
 {
+	if (!dev)


functions like this are used only internally
is it even possible accidentally pass an invalid dev here? I'm pretty sure if the user passes an invalid or NULL dev into any of the hid_* functions - there woud be an undefined behavior (crash) way before we get here in most places...

I don't know, I didn't discriminate when I added these.
In my opinion there shouldn't be any function without sanity checks.
It's not unthinkable that this was possible before I added the other checks after all.

In my opinion there shouldn't be any function without sanity checks.

That is actually debatable. Most of the checks in this PR are actually do more harm then help.
See my other comment below.

linux/hid.c

Megamouse · 2024-09-26T19:06:23Z

I think I made all the necessary changes.

Youw · 2024-09-27T09:05:45Z

libusb/hid.c

@@ -546,8 +558,10 @@ static void fill_device_info_usage(struct hid_device_info *cur_dev, libusb_devic
 		get_usage(hid_report_descriptor, res,  &page, &usage);
 	}

-	cur_dev->usage_page = page;
-	cur_dev->usage = usage;
+	if (cur_dev) {


early return here as well - it doesn't make sense to call this function with NULL cur_dev at all

windows/hid.c

Youw · 2024-09-27T09:25:50Z

linux/hid.c

+	if (!dev)
+		return -1;


all public API (specifically functions that accept hid_device * as first argument):

do not make sens to call with non-valid dev

should register an error string in case if anything goes wrong, so the user could use hid_error to check what went wrong in case if -1 is returned

I do not support having this checks for all public API functions (marked with HID_API_CALL/HID_API_EXPORT_CALL).
We always assume that the dev should be valid, and that is up to the user to make sure it is. (It is fairly easy to implement in other languages that wrap this library w/o any additional runtime checks by using e.g. class invariant).
The only exception is hid_close function - similar to free it should gracefully accept NULL-device.

All other functions the accept hid_device * as an argument called from public API function should use the same assumption, that the dev is valid and doesn't require extra checks.

Given that most of these functions already have sanity checks including hid_error for other parameters, I kept the checks and added report_global_error calls.

Youw · 2024-09-27T18:43:48Z

windows/hid.c

+	if (!dev) {
+		register_global_error(L"Device is NULL");
+		return -1;
+	}


this is 100% not a supported scenario and should not be handled explicitly in such way
pieces like this create too much "noise/distraction" in the code that actually makes the overall implementation worse than better

please revert

So you'd rather have a hard crash on this than an error that the user can actually catch...

Let me rephrase that (and that is not only my opinion, but rather shared experience of many developers/projects I've seen):
if I as a user messed up some implementation - I'd rather get a crash immediately (and have a chance to fix it immediatelly), that a silent error (often hidden due to lack of error-handling code) which is missed and then the real problem (the one that affects some business scenarios on your application) hits later when something slightly changed in a different part of my application.

I can remove it then. Although I would probably have to double check if there isn't an unguarded call to this.
I can make a separate PR with the obvious changes that don't need discussion (like the malloc ptr checks).

I can make a separate PR with the obvious changes that don't need discussion (like the malloc ptr checks).

Great idea, lets start with that.

Youw · 2024-10-01T07:10:06Z

NOTE: in #698 I've added some of those

Megamouse · 2024-10-04T20:38:00Z

If you tell me what to keep and what to remove I can adjust this soon