add better windows error handling #388

2022-02-24T09:14:34-06:00

Phyllostachys commented

2022-02-24 09:14:34 -06:00

(Migrated from github.com)

Oops, accidentally launched this PR before I intended. This PR merges the Windows messaging with what's already done in the Windows side of the library so that we can get better error reporting.

Youw commented

2022-02-25 02:48:40 -06:00

(Migrated from github.com)

Regarding global error:

From the hid_error documentation:

This function is thread-safe, and error messages are thread-local.

The only backend that tries to implement this correctly is hidraw.
Nevertheless it has an issue too (a memory leak under some conditions).

While I understand your best inentions of preparing this PR, I'd rather want to accept an implementation that strictly follows all requirements as per documentation: uses a thread-local storage of some kind, and correctly releases all the error strings when not required anymore.

Or we need to change initial requirement, to make it easier to implement.

I don't see a perfect solution as of yet.

The device error improvement I'll gladly accept.

Regarding _global error_: From the `hid_error` documentation: > This function is thread-safe, and error messages are thread-local. The only backend that _tries_ to implement this correctly is `hidraw`. Nevertheless it has an issue too (a memory leak under some conditions). While I understand your best inentions of preparing this PR, I'd rather want to accept an implementation that strictly follows all requirements as per documentation: uses a thread-local storage of some kind, and correctly releases all the error strings when not required anymore. Or we need to change initial requirement, to make it easier to implement. I don't see a perfect solution as of yet. --- The device error improvement I'll gladly accept.

Phyllostachys commented

2022-02-25 07:51:04 -06:00

(Migrated from github.com)

Hmm, yeah, that makes sense. I'll alter the PR to remove the global error handling.

I noticed when I tried adding the device error to the windows error that there was some heap corruption on freeing the memory via LocalFree() in free_hid_device(). I couldn't tell if it was introduced by me or the heap corruption was happening somewhere else and my changes exposed it somehow. I debugged in Visual Studios and traced the LocalFree()s on the error string storage and didn't see any double frees anywhere.

We've been having issues with a downstream library (hidapi-rs) where after a HID device reset, everything crashes. We have that library pinned to a certain version which seems to use hidapi 0.11.0 (0ec60c0).

Anyway, I'll remove the global error handling, do some more testing, and push the changes to this PR.

Hmm, yeah, that makes sense. I'll alter the PR to remove the global error handling. I noticed when I tried adding the device error to the windows error that there was some heap corruption on freeing the memory via `LocalFree()` in `free_hid_device()`. I couldn't tell if it was introduced by me or the heap corruption was happening somewhere else and my changes exposed it somehow. I debugged in Visual Studios and traced the `LocalFree()`s on the error string storage and didn't see any double frees anywhere. We've been having issues with a downstream library (`hidapi-rs`) where after a HID device reset, everything crashes. We have that library pinned to a certain version which seems to use hidapi 0.11.0 ([0ec60c0](https://github.com/libusb/hidapi/commit/0ec60c03cbe87cdbfb9fb199c7536fdcbc0a94b8)). Anyway, I'll remove the global error handling, do some more testing, and push the changes to this PR.

Phyllostachys commented

2022-02-28 08:10:30 -06:00

(Migrated from github.com)

Alright, removed global error messaging. I also had to switch the device specific error message to calloc and free since the Win32 LocalAlloc and LocalFree were doing some heap corruption (even though it's supposed to be thread local).

Alright, removed global error messaging. I also had to switch the device specific error message to `calloc` and `free` since the Win32 `LocalAlloc` and `LocalFree` were doing some heap corruption (even though it's supposed to be thread local).

Youw (Migrated from github.com) reviewed

2022-02-28 09:08:03 -06:00

Youw (Migrated from github.com) left a comment

I see two general issues with this implementation:

No early return in case if dev == NULL; if it is NULL (for whatever reason) - we would silently get a memory leak;
Double memory allocation (I guess no need to explain);

More comments regarding 2) :
Would be be willing to perform some additional refactoring?
I.e.: get rid of FORMAT_MESSAGE_ALLOCATE_BUFFER to use preallocated memory in the first place, and ~~use FORMAT_MESSAGE_ARGUMENT_ARRAY to make the formatting.~~

UPD: ok, after checking the doc, I see that we need FORMAT_MESSAGE_FROM_SYSTEM to have string representation of the rror code so the FORMAT_MESSAGE_FROM_STRING isn't a viable option, but still, calling FormatMessageW two times without the FORMAT_MESSAGE_ALLOCATE_BUFFER flag is enough to do both: concatenate the op/"error code message" and avoid double memory allocation.

I see two general issues with this implementation: 1) No early return in case if `dev == NULL`; if it is NULL (for whatever reason) - we would silently get a memory leak; 2) Double memory allocation (I guess no need to explain); More comments regarding 2) : Would be be willing to perform some additional refactoring? I.e.: get rid of `FORMAT_MESSAGE_ALLOCATE_BUFFER` to use preallocated memory in the first place, and ~~use `FORMAT_MESSAGE_ARGUMENT_ARRAY` to make the formatting.~~ UPD: ok, after checking the doc, I see that we need `FORMAT_MESSAGE_FROM_SYSTEM` to have string representation of the rror code so the `FORMAT_MESSAGE_FROM_STRING` isn't a viable option, but still, calling `FormatMessageW` two times without the `FORMAT_MESSAGE_ALLOCATE_BUFFER` flag is enough to do both: concatenate the `op`/"error code message" and avoid double memory allocation.

windows/hid.c

					
				@ -233,3 +243,3 @@

						NULL,

						GetLastError(),

						error_code,

						MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),

Does the cocumentation of FormatMessageW guarantees that the message will end with \r\n?

I don't like this approach (nor the previous one).

Technically correct would be:

while(msg_size > 0 && (msg[msg_size-1] == '\r' || msg[msg_size-1] == '\n'))
	msg_size--;

Does the cocumentation of `FormatMessageW` guarantees that the message will end with `\r\n`? I don't like this approach (nor the previous one). Technically correct would be: ``` while(msg_size > 0 && (msg[msg_size-1] == '\r' || msg[msg_size-1] == '\n')) msg_size--; ```

Phyllostachys commented

2022-03-03 09:14:33 -06:00

(Migrated from github.com)

Yes, these are good recommendations.

Phyllostachys commented

2022-03-03 10:03:32 -06:00

(Migrated from github.com)

The usage of these wide chars makes the concatenation annoying (can't just strcpy to the FormatMessageW buffer). Also, FormatMessageW requires a real buffer to write to in order to return the error string length. Seems I'm going to have to tinker with this more to get it right.

The usage of these wide chars makes the concatenation annoying (can't just `strcpy` to the `FormatMessageW` buffer). Also, `FormatMessageW` requires a real buffer to write to in order to return the error string length. Seems I'm going to have to tinker with this more to get it right.

Youw commented

2022-03-05 07:27:02 -06:00

(Migrated from github.com)

The usage of these wide chars makes the concatenation annoying (can't just strcpy to the FormatMessageW buffer)

I believe we can easily and safely replace the type for op to be const wchar_t*.

> The usage of these wide chars makes the concatenation annoying (can't just strcpy to the FormatMessageW buffer) I believe we can easily and safely replace the type for `op` to be `const wchar_t*`.

👍 1

Youw commented

2022-04-25 05:31:00 -05:00

(Migrated from github.com)

requires a real buffer to write to in order to return the error string length

That was a surprise for me. Most WinAPI functions allow passing NULL for buffer to get the required buffer size.

> requires a real buffer to write to in order to return the error string length That was a surprise for me. Most WinAPI functions allow passing NULL for buffer to get the required buffer size.

Youw commented

2022-04-25 06:20:21 -05:00

(Migrated from github.com)