Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add better windows error handling #388

Merged
merged 5 commits into from
May 2, 2022
Merged

add better windows error handling #388

merged 5 commits into from
May 2, 2022

Conversation

Phyllostachys
Copy link
Contributor

@Phyllostachys Phyllostachys commented Feb 24, 2022

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
Copy link
Member

Youw commented Feb 25, 2022

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
Copy link
Contributor Author

Phyllostachys commented Feb 25, 2022

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.

@Phyllostachys
Copy link
Contributor Author

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).

Copy link
Member

@Youw Youw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@mcuee mcuee added the Windows Related to Windows backend label Mar 1, 2022
@Phyllostachys
Copy link
Contributor Author

Yes, these are good recommendations.

@Phyllostachys
Copy link
Contributor Author

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
Copy link
Member

Youw commented Mar 5, 2022

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

@Youw
Copy link
Member

Youw commented Apr 25, 2022

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
Copy link
Member

Youw commented Apr 25, 2022

I took liberty to update the implementation on my own

@Youw Youw added this to In progress in v0.12 Release Apr 25, 2022
@Youw Youw merged commit 08563a0 into libusb:master May 2, 2022
v0.12 Release automation moved this from In progress to Done May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Related to Windows backend
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants