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

Fix HID device open failure handling #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthijskooijman
Copy link

When the ACM device could be opened to restart into the bootloader, but
the HID device was detected but failed to open (for example because of a
permission problem), this would not be detected and the code would
segfault because handle was NULL.

The code that was supposed to handle a NULL return value from hid_open
actually triggered only on a non-null value, and apparently also only
when the "looking for hid device" loop above had reached i = 10 (in
which case some error handling above would have bailed out already).

This changes the code to just check for a NULL return value, and bail
out in that case.

When the ACM device could be opened to restart into the bootloader, but
the HID device was detected but failed to open (for example because of a
permission problem), this would not be detected and the code would
segfault because `handle` was NULL.

The code that was supposed to handle a NULL return value from `hid_open`
actually triggered only on a *non-null* value, and apparently also only
when the "looking for hid device" loop above had reached i = 10 (in
which case some error handling above would have bailed out already).

This changes the code to just check for a NULL return value, and bail
out in that case.
@matthijskooijman
Copy link
Author

In the Arduino IDE (with the Arduino_Core_STM32 core), this looked like this:

+-----------------------------------------------------------------------+
|         HID-Flash v2.2.1 - STM32 HID Bootloader Flash Tool            |
|     (c)      2018 - Bruno Freitas       http://www.brunofreitas.com   |
|     (c) 2018-2019 - Vassilis Serasidis  https://www.serasidis.gr      |
|   Customized for STM32duino ecosystem   https://www.stm32duino.com    |
+-----------------------------------------------------------------------+

> Trying to open the [ttyACM0]...
> Toggling DTR...
> Searching for [1209:BEBA] device...
##
> [1209:BEBA] device is found !
> Sending <reset pages> command...
An error occurred while uploading the sketch

I noticed the segfault in dmesg:

[364192.188296] hid-flash[12130]: segfault at c ip 0000000000404687 sp 00007fff108c5db0 error 4 in hid-flash[402000+4000]
[364192.188318] Code: b6 00 0f b6 c0 89 45 f0 c7 45 ec 00 00 00 00 83 7d f0 00 75 11 48 83 45 d0 01 48 83 6d c8 01 c7 45 ec 01 00 00 00 48 8b 45 d8 <8b> 40 0c 
85 c0 7f 6a 48 8b 45 c8 0f b7 c8 48 8b 45 d8 8b 40 14 0f

@jalius
Copy link

jalius commented Apr 8, 2020

Hi. Any idea what permission may be missing? My device shows up properly in /dev/ttyACM0, but at hid_open there is a null handle.

@rogerclarkmelbourne
Copy link

rogerclarkmelbourne commented Jun 21, 2020

@matthijskooijman

Does the F4 version build for you ?

I'm getting errors about missing headers :-(

BTW.
This is nothing to do with your PR, but you seem to be the only one actively working on this, and I need to recompile it as I need to change the jump address to mimic another bootloader

@matthijskooijman
Copy link
Author

@rogerclarkmelbourne, I haven't tried compiling the bootloader itself, sorry. I just used the prebuilt version and noticed a bug in the hid-flash utility.

but you seem to be the only one actively working on this

That's overstating things, I was just testing a little bit to see if the core changes I made did not break the HID bootloader upload, I'm really not working on the bootloader itself (and have little interest in doing so, we use the builtin hardware bootloader).

@rogerclarkmelbourne
Copy link

@matthijskooijman

No worries. I contacted @Serasidis and he has pushed and update to fix the problem.

@jerkey
Copy link

jerkey commented Jul 21, 2023

I don't understand, why hasn't this been merged yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants