Use-after-free and heap-size-mismatch fix.

strike-kokhotnikov · strike-kokhotnikov · commit 641d296e0c05 · 2025-08-26T09:05:22.000-04:00
1. Removed redundant header dependencies to allow building vlogger and state_machine independently from the rest of the code.
2. Fixed logic of set log color output parameter.
3. Rewrote the functions read_file_to_int (src/vma/util/utils.cpp) and env_to_cpuset (src/vma/util/sys_vars.cpp) to enable running the code with ASAN.
4. Fixed a new/delete size mismatch in src/vma/dev/rfs*.
5. Fixed a use-after-free issue in timer_handler.
diff --git a/src/stats/stats_printer.cpp b/src/stats/stats_printer.cpp
@@ -11,8 +11,7 @@
 #include "vma/util/utils.h"
 #include "vma/util/vma_stats.h"
 #include "vma/lwip/tcp.h"
-#include "vma/vma_extra.h"
-#include "vma/util/sys_vars.h"
+#include "vma/util/vtypes.h"
 
 typedef enum {
 	e_K = 1024,
diff --git a/src/stats/stats_publisher.cpp b/src/stats/stats_publisher.cpp
@@ -102,7 +102,7 @@ void stats_data_reader::handle_timer_expired(void *ctx)
 
 void stats_data_reader::register_to_timer()
 {
-	m_timer_handler = g_p_event_handler_manager->register_timer_event(STATS_PUBLISHER_TIMER_PERIOD, g_p_stats_data_reader, PERIODIC_TIMER, 0);
+	m_timer_handler = g_p_event_handler_manager->register_timer_event(STATS_PUBLISHER_TIMER_PERIOD, this, PERIODIC_TIMER, 0);
 }
 
 void stats_data_reader::add_data_reader(void* local_addr, void* shm_addr, int size)
@@ -150,6 +150,8 @@ void vma_shmem_stats_open(vlog_levels_t** p_p_vma_log_level, uint8_t** p_p_vma_l
 	}
 	BULLSEYE_EXCLUDE_BLOCK_END
 
+	vlog_printf(VLOG_DEBUG,"%s:%d: Allocated g_p_stats_data_reader pointer as '%p'\n", __func__, __LINE__, g_p_stats_data_reader);
+
 	shmem_size = SHMEM_STATS_SIZE(safe_mce_sys().stats_fd_num_max);
 	buf = malloc(shmem_size);
 	if (buf == NULL)
@@ -277,8 +279,12 @@ void vma_shmem_stats_close()
 	g_sh_mem = NULL;
 	g_p_vlogger_level = NULL;
 	g_p_vlogger_details = NULL;
-	delete g_p_stats_data_reader;
-	g_p_stats_data_reader = NULL;
+	if (g_p_stats_data_reader != NULL) {
+		stats_data_reader* p = g_p_stats_data_reader;
+		g_p_stats_data_reader = NULL;
+		p->set_destroying_state(true);
+		delete p;
+	}
 }
 
 void vma_stats_instance_create_socket_block(socket_stats_t* local_stats_addr)
diff --git a/src/stats/stats_reader.cpp b/src/stats/stats_reader.cpp
@@ -15,9 +15,9 @@
 #include <fcntl.h>
 #include <errno.h>
 #include <list>
+#include <netinet/in.h>
 #include <signal.h>
-#include <getopt.h>		/* getopt()*/
-#include <errno.h>
+#include <getopt.h> /* getopt()*/
 #include <dirent.h>
 #include <string.h>
 #include <vector>
diff --git a/src/vlogger/vlogger.cpp b/src/vlogger/vlogger.cpp
@@ -17,8 +17,6 @@
 #include <fcntl.h>
 
 #include "utils/bullseye.h"
-#include "vma/util/utils.h"
-#include "vma/util/sys_vars.h"
 
 #define VLOG_DEFAULT_MODULE_NAME "VMA"
 #define VMA_LOG_CB_ENV_VAR "VMA_LOG_CB_FUNC_PTR"
@@ -31,7 +29,7 @@ vlog_levels_t* g_p_vlogger_level = NULL;
 uint8_t      g_vlogger_details = 0;
 uint8_t*     g_p_vlogger_details = NULL;
 uint32_t     g_vlogger_usec_on_startup = 0;
-bool         g_vlogger_log_in_colors = MCE_DEFAULT_LOG_COLORS;
+bool g_vlogger_log_in_colors = false;
 vma_log_cb_t g_vlogger_cb = NULL;
 
 namespace log_level
diff --git a/src/vma/dev/rfs.cpp b/src/vma/dev/rfs.cpp
@@ -120,7 +120,7 @@ rfs::~rfs()
 	delete[] m_sinks_list;
 
 	while (m_attach_flow_data_vector.size() > 0) {
-		delete m_attach_flow_data_vector.back();
+		free(m_attach_flow_data_vector.back());
 		m_attach_flow_data_vector.pop_back();
 	}
 }
diff --git a/src/vma/dev/rfs.h b/src/vma/dev/rfs.h
@@ -8,6 +8,8 @@
 #ifndef RFS_H
 #define RFS_H
 
+#include <stdlib.h>
+#include <type_traits>
 #include <vector>
 
 #include "vma/ib/base/verbs_extra.h"
@@ -213,6 +215,13 @@ class rfs
 	virtual bool 		rx_dispatch_packet(mem_buf_desc_t* p_rx_wc_buf_desc, void* pv_fd_ready_array) = 0;
 
 protected:
+        template <class T, typename ...Args>
+        T * new_malloc(Args ... args) {
+		static_assert(std::is_trivially_destructible<T>::value == true);
+		void * p = aligned_alloc(alignof(T), sizeof(T));
+		return new(p) T(args...);
+        }
+
 	flow_tuple		m_flow_tuple;
 	ring_slave*		m_p_ring;
 	rfs_rule_filter*	m_p_rule_filter;
diff --git a/src/vma/dev/rfs_mc.cpp b/src/vma/dev/rfs_mc.cpp
@@ -56,7 +56,7 @@ bool rfs_mc::prepare_flow_spec()
 #ifdef DEFINED_IBV_FLOW_SPEC_IB
 				attach_flow_data_ib_v1_t*  attach_flow_data_ib_v1 = NULL;
 
-				attach_flow_data_ib_v1 = new attach_flow_data_ib_v1_t(p_ring->m_p_qp_mgr);
+				attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_v1_t>(p_ring->m_p_qp_mgr);
 
 				uint8_t dst_gid[16];
 				create_mgid_from_ipv4_mc_ip(dst_gid, p_ring->m_p_qp_mgr->get_partiton(), m_flow_tuple.get_dst_ip());
@@ -70,7 +70,7 @@ bool rfs_mc::prepare_flow_spec()
 #endif
 			}
 
-			attach_flow_data_ib_v2 = new attach_flow_data_ib_v2_t(p_ring->m_p_qp_mgr);
+			attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_v2_t>(p_ring->m_p_qp_mgr);
 
 			ibv_flow_spec_ipv4_set(&(attach_flow_data_ib_v2->ibv_flow_attr.ipv4),
 						m_flow_tuple.get_dst_ip(),
@@ -88,7 +88,7 @@ bool rfs_mc::prepare_flow_spec()
 			{
 			attach_flow_data_eth_ipv4_tcp_udp_t*  attach_flow_data_eth = NULL;
 
-			attach_flow_data_eth = new attach_flow_data_eth_ipv4_tcp_udp_t(p_ring->m_p_qp_mgr);
+			attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);
 
 			uint8_t dst_mac[6];
 			create_multicast_mac_from_ip(dst_mac, m_flow_tuple.get_dst_ip());
diff --git a/src/vma/dev/rfs_uc.cpp b/src/vma/dev/rfs_uc.cpp
@@ -59,7 +59,7 @@ bool rfs_uc::prepare_flow_spec()
 			if (0 == p_ring->m_p_qp_mgr->get_underly_qpn()) {
 				attach_flow_data_ib_ipv4_tcp_udp_v1_t* attach_flow_data_ib_v1 = NULL;
 
-				attach_flow_data_ib_v1 = new attach_flow_data_ib_ipv4_tcp_udp_v1_t(p_ring->m_p_qp_mgr);
+				attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v1_t>(p_ring->m_p_qp_mgr);
 				ibv_flow_spec_ib_set_by_dst_qpn(&(attach_flow_data_ib_v1->ibv_flow_attr.ib),
 							htonl(((IPoIB_addr*)p_ring->m_p_l2_addr)->get_qpn()));
 				p_ipv4 = &(attach_flow_data_ib_v1->ibv_flow_attr.ipv4);
@@ -68,7 +68,7 @@ bool rfs_uc::prepare_flow_spec()
 				break;
 			}
 #endif
-			attach_flow_data_ib_v2 = new attach_flow_data_ib_ipv4_tcp_udp_v2_t(p_ring->m_p_qp_mgr);
+			attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v2_t>(p_ring->m_p_qp_mgr);
 
 			p_ipv4 = &(attach_flow_data_ib_v2->ibv_flow_attr.ipv4);
 			p_tcp_udp = &(attach_flow_data_ib_v2->ibv_flow_attr.tcp_udp);
@@ -77,7 +77,7 @@ bool rfs_uc::prepare_flow_spec()
 			}
 		case VMA_TRANSPORT_ETH:
 			{
-			attach_flow_data_eth = new attach_flow_data_eth_ipv4_tcp_udp_t(p_ring->m_p_qp_mgr);
+			attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);
 
 			ibv_flow_spec_eth_set(&(attach_flow_data_eth->ibv_flow_attr.eth),
 					p_ring->m_p_l2_addr->get_address(),
diff --git a/src/vma/dev/time_converter_ptp.h b/src/vma/dev/time_converter_ptp.h
@@ -9,6 +9,7 @@
 #define TIME_CONVERTER_PTP_H
 
 #include <infiniband/verbs.h>
+#include "vma/ib/base/verbs_extra.h"
 #include "vma/event/timer_handler.h"
 #include <vma/util/sys_vars.h>
 #include "time_converter.h"
diff --git a/src/vma/event/delta_timer.cpp b/src/vma/event/delta_timer.cpp
@@ -200,19 +200,11 @@ void timer::process_registered_timers()
 	timer_node_t* iter = m_list_head;
 	timer_node_t* next_iter;
 	while (iter && (iter->delta_time_msec == milliseconds(0))) {
-		tmr_logfuncall("timer expired on %p", iter->handler);
-
-		/* Special check is need to protect
-		 * from using destroyed object pointed by handler
-		 * See unregister_timer_event()
-		 * Object can be destoyed from another thread (lock protection)
-		 * and from current thread (lock and lock count condition)
-		 */
-		if (iter->handler &&
-			!iter->lock_timer.trylock() &&
-			(1 == iter->lock_timer.is_locked_by_me())) {
-			iter->handler->handle_timer_expired(iter->user_data);
-			iter->lock_timer.unlock();
+		timer_handler * handler = iter->handler.load();
+		tmr_logfuncall("timer expired on %p", handler);
+
+		if (handler) {
+			handler->safe_handle_timer_expired(iter->user_data);
 		}
 		next_iter = iter->next;
 
@@ -225,13 +217,13 @@ void timer::process_registered_timers()
 			break;
 
 		case ONE_SHOT_TIMER:
-			remove_timer(iter, iter->handler);
+			remove_timer(iter, handler);
 			break;
 
 		BULLSEYE_EXCLUDE_BLOCK_START
 		case INVALID_TIMER:
 		default:
-			tmr_logwarn("invalid timer expired on %p", iter->handler);
+			tmr_logwarn("invalid timer expired on %p", handler);
 			break;
 		}
 		BULLSEYE_EXCLUDE_BLOCK_END
diff --git a/src/vma/event/delta_timer.h b/src/vma/event/delta_timer.h
@@ -8,6 +8,7 @@
 #ifndef DELTA_TIMER_H
 #define DELTA_TIMER_H
 
+#include <atomic>
 #include <chrono>
 #include "utils/lock_wrapper.h"
 
@@ -32,13 +33,8 @@ struct timer_node_t {
 	std::chrono::milliseconds            delta_time_msec;
 	/* the orig timer requested (saved in order to re-register periodic timers) */
 	std::chrono::milliseconds            orig_time_msec;
-	/* control thread-safe access to handler. Recursive because unregister_timer_event()
-	 * can be called from handle_timer_expired()
-	 * that is under trylock() inside process_registered_timers
-	 */
-	lock_spin_recursive     lock_timer;
 	/* link to the context registered */
-	timer_handler*          handler;
+	std::atomic<timer_handler*> handler;
 	void*                   user_data;
 	timers_group*           group;
 	timer_req_type_t        req_type;
diff --git a/src/vma/event/event_handler_manager.cpp b/src/vma/event/event_handler_manager.cpp
@@ -74,9 +74,6 @@ void* event_handler_manager::register_timer_event(int timeout_msec, timer_handle
 	}
 	BULLSEYE_EXCLUDE_BLOCK_END
 
-	timer_node_t* timer_node = (timer_node_t*)node;
-	timer_node->lock_timer=lock_spin_recursive("timer");
-
 	reg_action_t reg_action;
 	memset(&reg_action, 0, sizeof(reg_action));
 	reg_action.type = REGISTER_TIMER;
@@ -116,30 +113,19 @@ void event_handler_manager::unregister_timer_event(timer_handler* handler, void*
 	reg_action.type = UNREGISTER_TIMER;
 	reg_action.info.timer.handler = handler;
 	reg_action.info.timer.node = node;
-
-	/* Special protection is needed to avoid scenario when deregistration is done
-	 * during timer_handler object destruction, timer node itself is not removed
-	 * and time for this timer node is expired. In this case there is no guarantee
-	 * to operate with timer_handler object.
-	 * See timer::process_registered_timers()
-	 * Do just lock() to protect timer_handler inside process_registered_timers()
-	 */
-	if (node) {
-		timer_node_t* timer_node = (timer_node_t*)node;
-		timer_node->lock_timer.lock();
-	}
-
 	post_new_reg_action(reg_action);
 }
 
 void event_handler_manager::unregister_timers_event_and_delete(timer_handler* handler)
 {
 	evh_logdbg("timer handler '%p'", handler);
-	reg_action_t reg_action;
-	memset(&reg_action, 0, sizeof(reg_action));
-	reg_action.type = UNREGISTER_TIMERS_AND_DELETE;
-	reg_action.info.timer.handler = handler;
-	post_new_reg_action(reg_action);
+	if( handler != nullptr && !handler->set_destroying_state()) {
+		reg_action_t reg_action;
+		memset(&reg_action, 0, sizeof(reg_action));
+		reg_action.type = UNREGISTER_TIMERS_AND_DELETE;
+		reg_action.info.timer.handler = handler;
+		post_new_reg_action(reg_action);
+	}
 }
 
 void event_handler_manager::register_ibverbs_event(int fd, event_handler_ibverbs *handler, 
diff --git a/src/vma/event/timer_handler.h b/src/vma/event/timer_handler.h
@@ -8,16 +8,70 @@
 #ifndef TIMER_HANDLER_H
 #define TIMER_HANDLER_H
 
+#include <atomic>
+
+#include "vlogger/vlogger.h"
+#include "utils/lock_wrapper.h"
+
 /**
  * simple timer notification.
  * Any class that inherit timer_handler should also inherit cleanable_obj, and use clean_obj instead of delete.
  * It must implement the clean_obj method to delete the object from the internal thread.
  */
 class timer_handler
 {
+private:
+    lock_spin m_handle_mutex{"timer_handler"};
+    std::atomic<bool> m_destroy_in_progress{false};
+protected:
+    virtual void handle_timer_expired(void* user_data) = 0;
+
 public:
-	virtual ~timer_handler() {};
-	virtual void handle_timer_expired(void* user_data) = 0;
+    timer_handler() = default;
+
+    virtual ~timer_handler() {
+        if( !m_destroy_in_progress.load()) {
+            m_destroy_in_progress = true;
+            vlog_printf(VLOG_DEBUG, "Destroying timer_handler without destroy in progress.\n");
+	}
+        {
+            m_handle_mutex.lock();
+	    m_handle_mutex.unlock();
+        }
+    };
+
+    void safe_handle_timer_expired(void* user_data) {
+        if(!m_destroy_in_progress.load()) {
+            if (m_handle_mutex.trylock() == 0) {
+                handle_timer_expired(user_data);
+                m_handle_mutex.unlock();
+            }
+        }
+    }
+
+/**
+  * Sets the destroying state of the object to indicate that destruction is in progress.
+  *
+  * If `wait_for_handler` is set to true, this method will ensure that the mutex
+  * used for handling operations (m_handle_mutex) is acquired and released, effectively
+  * waiting for any pending handler operation to complete.
+  *
+  * @param wait_for_handler If true, waits for the handler mutex to ensure
+  *                         handler finish before proceeding.
+  *                         Defaults to false.
+  *
+  * @return The previous state of the destruction flag.
+  *         Returns true if a destruction process was already in progress before
+  *         this method was called, otherwise false.
+  */
+    bool set_destroying_state(bool wait_for_handler = false) {
+        bool result = m_destroy_in_progress.exchange(true);
+            if( wait_for_handler ) {
+                m_handle_mutex.lock();
+                m_handle_mutex.unlock();
+            }
+	    return result;
+    }
 };
 
 #endif
diff --git a/src/vma/sock/fd_collection.cpp b/src/vma/sock/fd_collection.cpp
@@ -635,7 +635,7 @@ void  fd_collection::handle_timer_expired(void* user_data)
 			if (si_tcp) {
 				//In case of TCP socket progress the TCP connection
 				fdcoll_logfunc("Call to handler timer of TCP socket:%d", (*itr)->get_fd());
-				si_tcp->handle_timer_expired(NULL);
+				si_tcp->safe_handle_timer_expired(NULL);
 			}
 			itr++;
 		}
diff --git a/src/vma/sock/sockinfo_tcp.cpp b/src/vma/sock/sockinfo_tcp.cpp
@@ -4789,8 +4789,9 @@ void tcp_timers_collection::handle_timer_expired(void* user_data)
 	NOT_IN_USE(user_data);
 	timer_node_t* iter = m_p_intervals[m_n_location];
 	while (iter) {
-		__log_funcall("timer expired on %p", iter->handler);
-		iter->handler->handle_timer_expired(iter->user_data);
+		timer_handler * handler = iter->handler.load();
+		__log_funcall("timer expired on %p", handler);
+		handler->safe_handle_timer_expired(iter->user_data);
 		iter = iter->next;
 	}
 	m_n_location = (m_n_location + 1) % m_n_intervals_size;
@@ -4850,7 +4851,7 @@ void tcp_timers_collection::remove_timer(timer_node_t* node)
 		}
 	}
 
-	__log_dbg("TCP timer handler [%p] was removed", node->handler);
+	__log_dbg("TCP timer handler [%p] was removed", node->handler.load());
 
 	free(node);
 }
diff --git a/src/vma/util/sys_vars.cpp b/src/vma/util/sys_vars.cpp
diff --git a/src/vma/util/sys_vars.h b/src/vma/util/sys_vars.h
diff --git a/src/vma/util/utils.cpp b/src/vma/util/utils.cpp

Original file line number	Diff line number	Diff line change
`@@ -120,7 +120,7 @@ rfs::~rfs()`
`120`	`120`	`delete[] m_sinks_list;`
`121`	`121`
`122`	`122`	`while (m_attach_flow_data_vector.size() > 0) {`
`123`		`- delete m_attach_flow_data_vector.back();`
	`123`	`+ free(m_attach_flow_data_vector.back());`
`124`	`124`	`m_attach_flow_data_vector.pop_back();`
`125`	`125`	`}`
`126`	`126`	`}`