[GitHub] [incubator-nuttx] gustavonihei commented on a diff in pull request #6322: ESP32/ESP32-C3: Fix leak of semaphores created by Wi-Fi kernel thread

[GitBox <gi...@apache.org>]

gustavonihei commented on code in PR #6322:
URL: https://github.com/apache/incubator-nuttx/pull/6322#discussion_r880902276


##########
arch/risc-v/src/esp32c3/esp32c3_wifi_adapter.c:
##########
@@ -1338,41 +1334,40 @@ static int IRAM_ATTR wifi_is_in_isr(void)
 
 static void *esp_thread_semphr_get(void)
 {
+  static int wifi_task_key = -1;
   int ret;
-  int i;
   void *sem;
-  struct tcb_s *tcb = this_task();
-  struct task_group_s *group = tcb->group;
 
-  for (i = 0; i < CONFIG_SCHED_EXIT_MAX; i++)
+  if (wifi_task_key < 0)
     {
-      if (group->tg_exit[i].func.on == esp_thread_semphr_free)
+      ret = task_tls_alloc(esp_thread_semphr_free);
+      if (ret < 0)
         {
-          break;
+          wlerr("Failed to create task local key\n");
+          return NULL;
         }
+
+      wifi_task_key = ret;
     }
 
-  if (i >= CONFIG_SCHED_EXIT_MAX)
+  sem = (void *)task_tls_get_value(wifi_task_key);

Review Comment:
   > I'm just thinking that if the kthread exits and then started for the second time, then `wifi_task_key` will be non-null, but `task_tls_get_value(wifi_task_key);` will return non-null pointer that is already freed by `esp_thread_semphr_free`.
   
   In this proposed scenario where a given kernel thread is started a second time, `task_tls_get_value` will index a newly created `task_info_s` struct. So even though the key value has not been reset due to static storage duration, the `task_tls_get_value` couldn't do much for preventing this wrong usage of the `tlsindex` key.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org