From fa5ac39d6bfb5961cf1fb48c13e31e96c5665bdd Mon Sep 17 00:00:00 2001 From: Frédéric Desbiens Date: Thu, 4 Jun 2026 16:57:57 -0400 Subject: fix(hid): fixed memory leak and double-free in keyboard/mouse/remote_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> --- common/usbx_host_classes/src/ux_host_class_hid_deactivate.c | 11 ++++++++--- common/usbx_host_classes/src/ux_host_class_hid_entry.c | 9 +++++++-- .../src/ux_host_class_hid_keyboard_activate.c | 6 ++++++ .../src/ux_host_class_hid_keyboard_deactivate.c | 3 +++ .../usbx_host_classes/src/ux_host_class_hid_keyboard_entry.c | 5 ++++- .../usbx_host_classes/src/ux_host_class_hid_mouse_activate.c | 6 ++++++ .../src/ux_host_class_hid_mouse_deactivate.c | 3 +++ common/usbx_host_classes/src/ux_host_class_hid_mouse_entry.c | 5 ++++- .../src/ux_host_class_hid_remote_control_activate.c | 3 +++ .../src/ux_host_class_hid_remote_control_deactivate.c | 3 +++ 10 files changed, 47 insertions(+), 7 deletions(-) diff --git a/common/usbx_host_classes/src/ux_host_class_hid_deactivate.c b/common/usbx_host_classes/src/ux_host_class_hid_deactivate.c index d3db9ac..888cc12 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_deactivate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_deactivate.c @@ -144,9 +144,14 @@ UINT status; { hid -> ux_host_class_hid_client -> ux_host_class_hid_client_handler(&hid_client_command); - /* Free the per-instance client copy allocated in _ux_host_class_hid_client_search. */ - _ux_utility_memory_free(hid -> ux_host_class_hid_client); - hid -> ux_host_class_hid_client = UX_NULL; + /* Free the per-instance client copy allocated in _ux_host_class_hid_client_search. + Handlers for keyboard/mouse/remote_control null hid_client after freeing their own + combined allocation; simple clients leave hid_client pointing at the per-instance copy. */ + if (hid -> ux_host_class_hid_client != UX_NULL) + { + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + hid -> ux_host_class_hid_client = UX_NULL; + } } /* Clean all the HID memory fields. */ diff --git a/common/usbx_host_classes/src/ux_host_class_hid_entry.c b/common/usbx_host_classes/src/ux_host_class_hid_entry.c index 9acc9d3..d58b58e 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_entry.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_entry.c @@ -456,8 +456,13 @@ UINT status; /* Error. */ if (status < UX_STATE_NEXT) { - _ux_utility_memory_free(hid -> ux_host_class_hid_client); - hid -> ux_host_class_hid_client = UX_NULL; + + /* Free per-instance client copy if not already freed by the ACTIVATE_WAIT handler. */ + if (hid -> ux_host_class_hid_client != UX_NULL) + { + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + hid -> ux_host_class_hid_client = UX_NULL; + } hid -> ux_host_class_hid_status = UX_DEVICE_ENUMERATION_FAILURE; hid -> ux_host_class_hid_enum_state = UX_HOST_CLASS_HID_ENUM_ERROR; return; diff --git a/common/usbx_host_classes/src/ux_host_class_hid_keyboard_activate.c b/common/usbx_host_classes/src/ux_host_class_hid_keyboard_activate.c index af2d5c6..5deebcc 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_keyboard_activate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_keyboard_activate.c @@ -347,6 +347,9 @@ UX_HOST_CLASS_HID_FIELD *field; - _periodic_report_start() */ keyboard_instance -> ux_host_class_hid_keyboard_enum_state = UX_STATE_WAIT; + /* Free the per-instance client copy from client_search, as we now use our own embedded copy. */ + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + /* It's fine, replace client with our copy. */ hid -> ux_host_class_hid_client = hid_client; return(status); @@ -402,6 +405,9 @@ UX_HOST_CLASS_HID_FIELD *field; if (status == UX_SUCCESS) { + /* Free the per-instance client copy from client_search, as we now use our own embedded copy. */ + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + /* It's fine, replace client copy. */ hid -> ux_host_class_hid_client = hid_client; diff --git a/common/usbx_host_classes/src/ux_host_class_hid_keyboard_deactivate.c b/common/usbx_host_classes/src/ux_host_class_hid_keyboard_deactivate.c index 942d988..88bfc57 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_keyboard_deactivate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_keyboard_deactivate.c @@ -123,6 +123,9 @@ UINT status = UX_SUCCESS; /* Now free the instance memory. */ _ux_utility_memory_free(hid_client -> ux_host_class_hid_client_local_instance); + /* Signal to _ux_host_class_hid_deactivate that the client memory has been freed. */ + hid -> ux_host_class_hid_client = UX_NULL; + /* Return completion status. */ return(status); } diff --git a/common/usbx_host_classes/src/ux_host_class_hid_keyboard_entry.c b/common/usbx_host_classes/src/ux_host_class_hid_keyboard_entry.c index 4e4582e..3c863ae 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_keyboard_entry.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_keyboard_entry.c @@ -226,9 +226,12 @@ UINT status; if (keyboard -> ux_host_class_hid_keyboard_usage_array) _ux_utility_memory_free(keyboard -> ux_host_class_hid_keyboard_usage_array); - /* Free instance. */ + /* Free instance (= combined allocation; hid_client is embedded inside it). */ _ux_utility_memory_free(keyboard); + /* Signal to entry.c that the per-instance client has been freed. */ + hid -> ux_host_class_hid_client = UX_NULL; + return(UX_STATE_ERROR); } diff --git a/common/usbx_host_classes/src/ux_host_class_hid_mouse_activate.c b/common/usbx_host_classes/src/ux_host_class_hid_mouse_activate.c index ec49be5..723831c 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_mouse_activate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_mouse_activate.c @@ -128,6 +128,9 @@ UINT status; } + /* Free the per-instance client copy from client_search, as we now use our own embedded copy. */ + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + /* Use our copy of client. */ hid -> ux_host_class_hid_client = hid_client; return(status); @@ -182,6 +185,9 @@ UINT status; if (status == UX_SUCCESS) { + /* Free the per-instance client copy from client_search, as we now use our own embedded copy. */ + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + /* Use our copy of client. */ hid -> ux_host_class_hid_client = hid_client; diff --git a/common/usbx_host_classes/src/ux_host_class_hid_mouse_deactivate.c b/common/usbx_host_classes/src/ux_host_class_hid_mouse_deactivate.c index cc2206c..760b41e 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_mouse_deactivate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_mouse_deactivate.c @@ -96,6 +96,9 @@ UINT status; /* Now free the instance memory. */ _ux_utility_memory_free(hid_client -> ux_host_class_hid_client_local_instance); + /* Signal to _ux_host_class_hid_deactivate that the client memory has been freed. */ + hid -> ux_host_class_hid_client = UX_NULL; + /* Return completion status. */ return(status); } diff --git a/common/usbx_host_classes/src/ux_host_class_hid_mouse_entry.c b/common/usbx_host_classes/src/ux_host_class_hid_mouse_entry.c index 141651a..f412304 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_mouse_entry.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_mouse_entry.c @@ -207,9 +207,12 @@ UX_HOST_CLASS_HID_REPORT_CALLBACK call_back; /* Detach instance. */ hid_client -> ux_host_class_hid_client_local_instance = UX_NULL; - /* Free instance. */ + /* Free instance (= combined allocation; hid_client is embedded inside it). */ _ux_utility_memory_free(mouse); + /* Signal to entry.c that the per-instance client has been freed. */ + hid -> ux_host_class_hid_client = UX_NULL; + return(UX_STATE_ERROR); } diff --git a/common/usbx_host_classes/src/ux_host_class_hid_remote_control_activate.c b/common/usbx_host_classes/src/ux_host_class_hid_remote_control_activate.c index c353fce..0c79d0e 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_remote_control_activate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_remote_control_activate.c @@ -143,6 +143,9 @@ UINT status = UX_SUCCESS; if (status == UX_SUCCESS) { + /* Free the per-instance client copy from client_search, as we now use our own embedded copy. */ + _ux_utility_memory_free(hid -> ux_host_class_hid_client); + /* Use out copy of client. */ hid -> ux_host_class_hid_client = hid_client; diff --git a/common/usbx_host_classes/src/ux_host_class_hid_remote_control_deactivate.c b/common/usbx_host_classes/src/ux_host_class_hid_remote_control_deactivate.c index 1db2e56..f9b4853 100644 --- a/common/usbx_host_classes/src/ux_host_class_hid_remote_control_deactivate.c +++ b/common/usbx_host_classes/src/ux_host_class_hid_remote_control_deactivate.c @@ -103,6 +103,9 @@ UINT status; /* Now free the instance memory. */ _ux_utility_memory_free(hid_client -> ux_host_class_hid_client_local_instance); + /* Signal to _ux_host_class_hid_deactivate that the client memory has been freed. */ + hid -> ux_host_class_hid_client = UX_NULL; + /* Return completion status. */ return(status); } -- cgit v1.3.1