fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(hid): Store client instance per-device instead of per-class - #262

Merged
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance
Jun 4, 2026
Merged

fix(hid): Store client instance per-device instead of per-class#262
fdesbiens merged 2 commits into
eclipse-threadx:devfrom
kajteklau:fix/hid-shared-client-instance

Conversation

@kajteklau

Copy link
Copy Markdown
Contributor

USBX HID Shared Client Instance Pointer Bug

Target Repo

eclipse-threadx/usbx (MIT license, Category A Simple Contribution)


Symptom

This bug surfaces when a user plugs in two or more HID devices of the same class to the hub tree. Removing any one HID device of a given type (keyboard or mouse) causes all remaining devices of that type to stop reporting input. Callbacks write into freed memory.


Background: The Global Client Table

At startup, the application registers HID client types (keyboard, mouse) by calling ux_host_class_hid_client_register(). This function allocates a single flat array of UX_HOST_CLASS_HID_CLIENT structs, stored at class->ux_host_class_client on the UX_HOST_CLASS container for HID. In this application there are two entries:

class->ux_host_class_client -> [ [0]: keyboard handler | local_instance ]
[ [1]: mouse handler | local_instance ]

Each entry holds a handler function pointer (e.g. ux_host_class_hid_keyboard_entry) and a VOID *ux_host_class_hid_client_local_instance field intended to point to the per-device instance (e.g. UX_HOST_CLASS_HID_KEYBOARD).


Original Code: Device Activation Sequence

When a USB HID device enumerates, the middleware creates a per-interface UX_HOST_CLASS_HID struct (hid) and calls _ux_host_class_hid_client_search(hid). This function:

  1. Iterates the global client table to find a matching client (by HID usage page/usage).
  2. Stores a direct pointer to the matching table entry on the HID instance:
    hid->ux_host_class_hid_client=hid_client; // points into the shared table
  3. Calls the client's activate handler (e.g. _ux_host_class_hid_keyboard_activate).

The activate handler then:

  1. Reads the client pointer back: hid_client = hid->ux_host_class_hid_client
  2. Allocates a new UX_HOST_CLASS_HID_KEYBOARD struct (keyboard_instance)
  3. Stores it on the shared table entry:
    hid_client->ux_host_class_hid_client_local_instance= (VOID*)keyboard_instance;

The problem: hid_client points to the shared table entry. Every keyboard shares the same entry. The local_instance field is a single pointer — the last keyboard to activate overwrites it.


Original Code: Device Deactivation Sequence

When a device is unplugged, _ux_host_class_hid_keyboard_deactivate() runs:

  1. Reads the client pointer: hid_client = hid->ux_host_class_hid_client (shared entry)
  2. Reads the keyboard instance: keyboard_instance = hid_client->local_instance
  3. Frees the keyboard instance's thread, semaphore, buffers, and the instance itself.

The deactivation has no other way to find the keyboard instance. The address was only
ever stored in one place: hid_client->local_instance on the shared table entry.


Failure Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> shared_entry.local_instance = keyboard_instance_A
2. Keyboard B activates
-> shared_entry.local_instance = keyboard_instance_B (overwrites A)
State: hid_A->hid_client -> shared_entry -> keyboard_instance_B
hid_B->hid_client -> shared_entry -> keyboard_instance_B
keyboard_instance_A exists in memory but nothing points to it
3. Keyboard A is unplugged
-> deactivate reads shared_entry.local_instance -> gets keyboard_instance_B
-> frees keyboard_instance_B (WRONG — this belongs to the still-connected keyboard)
State: hid_B is still active, interrupt endpoint still delivering data
hid_B's callback reads shared_entry.local_instance -> dangling pointer
keyboard_instance_A is leaked (never freed, nothing references it)

Keyboard B's interrupt reports continue to arrive, but the callback writes into freed memory. The result is silent data loss and memory leak.


Fix

Instead of storing a pointer to the shared table entry, allocate a per-instance copy of the client struct so each HID device gets its own local_instance pointer.

In _ux_host_class_hid_client_search() (lines 119–155):

UX_HOST_CLASS_HID_CLIENT*hid_client_instance;
hid_client_instance=_ux_utility_memory_allocate(..., sizeof(UX_HOST_CLASS_HID_CLIENT));
_ux_utility_memory_copy(hid_client_instance, hid_client, sizeof(UX_HOST_CLASS_HID_CLIENT));
hid_client_instance->ux_host_class_hid_client_local_instance=UX_NULL;
hid->ux_host_class_hid_client=hid_client_instance;

The copy inherits the handler function pointer and name from the shared entry but has its own local_instance field. local_instance is explicitly set to NULL so the copy doesn't carry a stale pointer from a previous activation of the same device type. The activate handler then writes the freshly allocated keyboard instance to this private
copy's local_instance.

In _ux_host_class_hid_deactivate(), the per-instance copy is freed after the client deactivate handler runs:

_ux_utility_memory_free(hid->ux_host_class_hid_client);
hid->ux_host_class_hid_client=UX_NULL;

Fixed Sequence (Two Keyboards, A and B)

1. Keyboard A activates
-> private_copy_A.local_instance = keyboard_instance_A
-> hid_A->hid_client = private_copy_A
2. Keyboard B activates
-> private_copy_B.local_instance = keyboard_instance_B
-> hid_B->hid_client = private_copy_B
3. Keyboard A is unplugged
-> deactivate reads private_copy_A.local_instance -> keyboard_instance_A (CORRECT)
-> frees keyboard_instance_A, then frees private_copy_A
Keyboard B is completely untouched.

Files Changed

  • common/usbx_host_classes/src/ux_host_class_hid_client_search.c (lines 119–155)
  • common/usbx_host_classes/src/ux_host_class_hid_deactivate.c

Memory Cost

One additional sizeof(UX_HOST_CLASS_HID_CLIENT) allocation (~40 bytes) per connected
HID device. Freed on device removal.

@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Hi, sorry I made a mistake when signing the ECA. It should be fixed now. Please let me know what can I do to contribute.

-- Kajtek

@fdesbiens

Copy link
Copy Markdown
Contributor

Hi @kajteklau.

The ECA passes. Thank you.

@rahmanih: Can you please review. I would like to ship this in the Q2 2026 release if possible.

@fdesbiens
fdesbiens changed the base branch from master to devMay 28, 2026 11:41
@fdesbiensfdesbiens moved this to In review in ThreadX RoadmapMay 28, 2026
Three issues fixed, discovered during maintainer review:
1. Memory leak in standalone activation error path (entry.c):
_ux_host_class_hid_client_activate_wait() set hid_client to NULL
without freeing the per-instance copy allocated in client_search.
The HID_ENUM_ERROR handler destroys the hid struct without freeing
hid_client, so the copy was leaked on every standalone activation
failure. Fixed by freeing hid_client before clearing it.
2. Variable declared inside if-block (client_search.c):
hid_client_instance was declared inside the if (status == UX_SUCCESS)
block, which is a C99 feature. USBX targets C89/C90 embedded
toolchains. Moved to the top of the function with other locals.
3. Trailing whitespace throughout both changed files:
The PR introduced trailing spaces on most comment-block lines.
Reverted all affected lines to their original whitespace.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fdesbiens

Copy link
Copy Markdown
Contributor

Thank you for this fix, @kajteklau. I addressed a few small issues I discovered while reviewing the PR. I am merging into dev now, and this will ship with our Q2 2026 release next week.

@fdesbiens
fdesbiens merged commit a5fd35f into eclipse-threadx:devJun 4, 2026
1 check passed
@github-project-automationgithub-project-automationBot moved this from In review to Done in ThreadX RoadmapJun 4, 2026
@kajteklau

Copy link
Copy Markdown
ContributorAuthor

Pleasure working with you guys. Thank you.

fdesbiens added a commit that referenced this pull request Jun 4, 2026
…control client lifecycle (#265)
PR #262 introduced per-instance UX_HOST_CLASS_HID_CLIENT allocation in
client_search.c. However, keyboard/mouse/remote_control activate handlers
already embed a UX_HOST_CLASS_HID_CLIENT inside their own combined
allocation (e.g. UX_HOST_CLASS_HID_CLIENT_KEYBOARD), override
hid->hid_client with the embedded copy, and then free the entire combined
struct (as 'keyboard_instance', the first field) during deactivation.
This created two bugs:
1. Memory leak: the per-instance copy from client_search was abandoned
when activate handlers replaced hid->hid_client with their embedded
copy.
2. Double-free / UX_MEMORY_CORRUPTED: deactivate.c freed hid->hid_client
after calling the handler, but for keyboard/mouse/remote_control the
deactivate handler had already freed the entire combined allocation
(which contains the embedded hid_client), causing a second free of a
pointer into the middle of a now-freed block.
Fix:
- keyboard/mouse/remote_control activate: free the per-instance copy from
client_search before overriding hid->hid_client with the embedded one.
- keyboard/mouse/remote_control deactivate: null hid->hid_client after
freeing the combined struct, signalling that cleanup is done.
- deactivate.c: re-check hid_client != NULL after calling the handler
before freeing; keyboard/mouse/remote_control will have nulled it,
simple clients will not.
- keyboard/mouse ACTIVATE_WAIT error paths (standalone): null
hid->hid_client after freeing the combined struct so that the generic
cleanup in entry.c skips the already-freed pointer.
- entry.c standalone ACTIVATE_WAIT error: guard the free with a NULL
check to safely handle both cases.
Discovered while investigating test failures introduced by PR #262.
All 430 tests pass after this fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@kajteklau@fdesbiens