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 <[email protected]>

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);
 }