issue: 4409403 Fix heap corruption since c73d96a

This commit fixes a critical race condition in timer management for TCP
sockets that was introduced in commit c73d96a.

The heap corruption was caused by a race condition between the timer
thread and socket destruction. Sockets could be deleted by the event
handler thread while still being referenced by the timer thread in the
timer collections, resulting in heap corruption when the timer thread
attempted to access the deleted memory.

In the original implementation, sockets were removed from timer
collections and deleted asynchronously without proper synchronization
with the timer processing thread.

Fix:
- Remove sockets from timer collections while still holding the socket
  lock, guaranteeing the timer thread cannot access sockets marked for
  deletion
- Create a simplified deletion path that doesn't attempt to access timer
  collections again after socket cleanup

Additionally, as an unrelated improvement, this patch fixes a lock leak
in the early return path of sockinfo_tcp::clean_socket_obj() where a
lock was acquired but not released when a socket was already marked as
cleaned.

The heap corruption stemmed from a fundamental architectural change that
separated socket objects from their timer management without providing
proper synchronization for the distributed socket lifecycle.

Signed-off-by: Tomer Cabouly <[email protected]>

Author: tomerdbz

src/core/event/event_handler_manager.cpp

@@ -156,12 +156,12 @@ void event_handler_manager::unregister_timers_event_and_delete(timer_handler *ha
     post_new_reg_action(reg_action);
 }
 
-void event_handler_manager::unregister_socket_timer_and_delete(sockinfo_tcp *sock_tcp)
+void event_handler_manager::unregister_socket_and_delete(sockinfo_tcp *sock_tcp)
 {
-    evh_logdbg("Unregistering TCP socket timer: %p", sock_tcp);
+    evh_logdbg("Deleting TCP socket: %p", sock_tcp);
     reg_action_t reg_action;
     memset(&reg_action, 0, sizeof(reg_action));
-    reg_action.type = UNREGISTER_TCP_SOCKET_TIMER_AND_DELETE;
+    reg_action.type = UNREGISTER_SOCKET_AND_DELETE;
     reg_action.info.timer.user_data = sock_tcp;
     post_new_reg_action(reg_action);
 }
@@ -421,8 +421,6 @@ const char *event_handler_manager::reg_action_str(event_action_type_e reg_action
     switch (reg_action_type) {
     case REGISTER_TCP_SOCKET_TIMER:
         return "REGISTER_TCP_SOCKET_TIMER";
-    case UNREGISTER_TCP_SOCKET_TIMER_AND_DELETE:
-        return "UNREGISTER_TCP_SOCKET_TIMER_AND_DELETE";
     case REGISTER_TIMER:
         return "REGISTER_TIMER";
     case UNREGISTER_TIMER:
@@ -441,6 +439,8 @@ const char *event_handler_manager::reg_action_str(event_action_type_e reg_action
         return "REGISTER_COMMAND";
     case UNREGISTER_COMMAND:
         return "UNREGISTER_COMMAND";
+    case UNREGISTER_SOCKET_AND_DELETE:
+        return "UNREGISTER_SOCKET_AND_DELETE";
         BULLSEYE_EXCLUDE_BLOCK_START
     default:
         return "UNKNOWN";
@@ -703,9 +703,9 @@ void event_handler_manager::handle_registration_action(reg_action_t &reg_action)
         sock = reinterpret_cast<sockinfo_tcp *>(reg_action.info.timer.user_data);
         sock->get_tcp_timer_collection()->add_new_timer(sock);
         break;
-    case UNREGISTER_TCP_SOCKET_TIMER_AND_DELETE:
+    case UNREGISTER_SOCKET_AND_DELETE:
         sock = reinterpret_cast<sockinfo_tcp *>(reg_action.info.timer.user_data);
-        sock->get_tcp_timer_collection()->remove_timer(sock);
+        // Just delete the socket without trying to remove from timer collection
         delete sock;
         break;
     case REGISTER_TIMER:

src/core/event/event_handler_manager.h

@@ -28,7 +28,7 @@ typedef std::map<void * /*event_handler_id*/, event_handler_rdma_cm * /*p_event_
 
 typedef enum {
     REGISTER_TCP_SOCKET_TIMER,
-    UNREGISTER_TCP_SOCKET_TIMER_AND_DELETE,
+    UNREGISTER_SOCKET_AND_DELETE,
     REGISTER_TIMER,
     WAKEUP_TIMER, /* NOT AVAILABLE FOR GROUPED TIMERS */
     UNREGISTER_TIMER,
@@ -167,7 +167,7 @@ class event_handler_manager : public wakeup_pipe {
     void unregister_timers_event_and_delete(timer_handler *handler);
 
     void register_socket_timer_event(sockinfo_tcp *sock_tcp);
-    void unregister_socket_timer_and_delete(sockinfo_tcp *sock_tcp);
+    void unregister_socket_and_delete(sockinfo_tcp *sock_tcp);
 
     void register_ibverbs_event(int fd, event_handler_ibverbs *handler, void *channel,
                                 void *user_data);

src/core/sock/sockinfo_tcp.cpp

@@ -595,18 +595,26 @@ void sockinfo_tcp::clean_socket_obj()
     lock_tcp_con();
 
     if (is_cleaned()) {
+        unlock_tcp_con();
         return;
     }
     m_is_cleaned = true;
 
+    // Important: Remove from timer collection WHILE STILL HOLDING THE LOCK
+    // This prevents the timer thread from processing this socket after it's marked for cleanup
+    if (is_timer_registered()) {
+        tcp_timers_collection *p_collection = get_tcp_timer_collection();
+        p_collection->remove_timer(this);
+    }
+
     unlock_tcp_con();
 
     event_handler_manager *p_event_mgr = get_event_mgr();
     bool delegated_timers_exit = g_b_exit &&
         (safe_mce_sys().tcp_ctl_thread == option_tcp_ctl_thread::CTL_THREAD_DELEGATE_TCP_TIMERS);
 
     if (p_event_mgr->is_running() && !delegated_timers_exit) {
-        p_event_mgr->unregister_socket_timer_and_delete(this);
+        p_event_mgr->unregister_socket_and_delete(this);
     } else {
         delete this;
     }