stats: Fix crash with dangling pointers. #1151

strike-kokhotnikov · 2025-11-14T14:34:03Z

Description

Sometimes it happens, that timer handlers are destroyed, but it still can run in timer thread. Current solution with lock_timer can't prevent this in case of closing the library. The new solution uses mutex for every handler and prevent execution of the handler in case of destruction, and pause destruction, when the handler is running.

What

Fix crash with dangling pointers in timers.

Change type

What kind of change does this PR introduce?

Check list

Code follows the style de facto guidelines of this project
Comments have been inserted in hard to understand places
Documentation has been updated (if necessary)
Test has been added (if possible)

svc-nbu-swx-media · 2025-11-14T14:35:08Z

Can one of the admins verify this patch?

greptile-apps · 2025-11-14T14:37:51Z

Greptile Overview

Greptile Summary

Replaces per-node recursive lock with per-handler mutex and atomic flag to prevent timer handlers from executing on destroyed objects during library shutdown. The solution moves synchronization from timer_node_t into the timer_handler base class, using std::atomic<timer_handler*> for handler pointers and a new safe_handle_timer_expired() method that checks destruction state before execution.

Key changes:

Added lock_spin m_handle_mutex and std::atomic<bool> m_destroy_in_progress to timer_handler base class
Changed timer_node_t::handler from raw pointer to std::atomic<timer_handler*>
Removed lock_spin_recursive lock_timer from timer_node_t structure
Destructor now waits for any in-progress handler execution by acquiring/releasing the mutex
All timer expiration call sites now use safe_handle_timer_expired() instead of direct handle_timer_expired()
Fixed stats_publisher.cpp to register timer with this instead of global pointer and properly call set_destroying_state() before deletion

Confidence Score: 4/5

This PR is safe to merge with minor concerns about silent handler skipping and potential ABA problem
The synchronization design is fundamentally sound - the combination of atomic handler pointers, destruction flag, and mutex provides proper protection against use-after-free. However, there's a subtle concern: safe_handle_timer_expired() uses trylock() which silently skips execution if the lock is held, potentially causing timers to be silently dropped under contention. Additionally, there's a theoretical ABA problem if a handler pointer is reused at the same address after deletion, though this is unlikely in practice. The changes are well-structured and consistently applied across all timer processing code paths.
Most critical file is src/vma/event/timer_handler.h where the core synchronization mechanism is implemented - verify the trylock behavior is acceptable for your use case

Important Files Changed

File Analysis

Filename	Score	Overview
src/vma/event/timer_handler.h	4/5	Adds mutex and atomic flag to base class for safe handler synchronization during destruction
src/vma/event/delta_timer.h	5/5	Changes handler pointer to atomic, removes recursive lock from timer_node_t structure
src/vma/event/delta_timer.cpp	5/5	Updates timer processing to use atomic loads and safe_handle_timer_expired for thread-safe execution
src/vma/event/event_handler_manager.cpp	5/5	Removes lock_timer initialization, adds set_destroying_state check in unregister_timers_event_and_delete
src/vma/sock/sockinfo_tcp.cpp	5/5	Adds null check and uses safe_handle_timer_expired in tcp_timers_collection
src/stats/stats_publisher.cpp	5/5	Fixes timer registration to use `this` instead of global, adds proper destruction with set_destroying_state

Sequence Diagram

sequenceDiagram
    participant App as Application Thread
    participant Timer as Timer Thread
    participant Handler as timer_handler Object
    participant Node as timer_node_t
    
    Note over App,Node: Registration Phase
    App->>Timer: register_timer_event(handler)
    Timer->>Node: allocate timer_node_t
    Timer->>Node: handler.store(handler_ptr)
    
    Note over App,Node: Normal Timer Expiration
    Timer->>Timer: process_registered_timers()
    Timer->>Node: handler_ptr = handler.load()
    Timer->>Handler: safe_handle_timer_expired(data)
    Handler->>Handler: trylock() m_handle_mutex
    Handler->>Handler: check m_destroy_in_progress
    alt Lock acquired and not destroying
        Handler->>Handler: handle_timer_expired(data)
        Handler->>Handler: unlock()
    else Lock failed or destroying
        Handler->>Timer: skip execution
    end
    
    Note over App,Node: Destruction Phase
    App->>Handler: set_destroying_state(true)
    Handler->>Handler: m_destroy_in_progress.exchange(true)
    App->>Timer: unregister_timers_event_and_delete()
    Timer->>Timer: remove_all_timers(handler)
    Timer->>Node: handler.store(nullptr)
    Timer->>Handler: delete handler
    Handler->>Handler: ~timer_handler()
    Handler->>Handler: lock() m_handle_mutex (waits if timer running)
    Handler->>Handler: unlock()
    Note over Handler: Safe to destroy - no timers running

greptile-apps

Additional Comments (7)

src/vma/event/delta_timer.cpp, line 97 (link)

syntax: comparing atomic pointer without .load() - should be node->handler.load() == handler
src/vma/event/delta_timer.cpp, line 105 (link)

syntax: comparing atomic pointer without .load() - should be node->handler.load() != handler
src/vma/event/delta_timer.cpp, line 112 (link)

syntax: assigning to atomic pointer without .store() - should be node->handler.store(NULL) or node->handler.store(nullptr)
src/vma/event/delta_timer.cpp, line 128 (link)

syntax: comparing atomic pointer without .load() - should be node->handler.load() == handler
src/vma/event/delta_timer.cpp, line 133 (link)

syntax: comparing atomic pointer without .load() - should be node_tmp->handler.load() != handler
src/vma/event/delta_timer.cpp, line 139 (link)

syntax: assigning to atomic pointer without .store() - should be node_tmp->handler.store(nullptr)
src/vma/event/delta_timer.cpp, line 57 (link)

syntax: assigning to atomic pointer without .store() - should be node->handler.store(handler)

_{10 files reviewed, 8 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-14T14:37:50Z

src/vma/event/timer_handler.h

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


logic: race condition: checking m_destroy_in_progress and trylock() are not atomic - thread can pass the check but destructor can start before trylock() succeeds, allowing handle_timer_expired() to run on partially destroyed object. the check should happen after acquiring the lock:

Suggested change

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

}

}

}

void safe_handle_timer_expired(void* user_data) {

if (m_handle_mutex.trylock() == 0) {

if(!m_destroy_in_progress.load()) {

handle_timer_expired(user_data);

}

m_handle_mutex.unlock();

}

}

greptile-apps

_{10 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-14T14:55:09Z

src/vma/event/timer_handler.h

+				m_handle_mutex.unlock();
+			}
+		}
+	}


syntax: extra closing brace causes syntax error - remove this line

Suggested change

}

greptile-apps

Additional Comments (1)

src/vma/sock/sockinfo_tcp.cpp, line 4814 (link)

syntax: atomic assignment requires .store() - direct assignment to std::atomic may not be thread-safe

_{10 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-14T15:02:39Z

src/vma/sock/sockinfo_tcp.cpp

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


logic: missing null check - handler can be null if timer was removed concurrently

Suggested change

handler->safe_handle_timer_expired(iter->user_data);

if (handler) {

handler->safe_handle_timer_expired(iter->user_data);

}

greptile-apps

_{10 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-14T15:12:53Z

src/vma/event/timer_handler.h

+				}
+				m_handle_mutex.unlock();
+			}
+    }


style: inconsistent indentation - uses spaces instead of tabs like the rest of the file

Suggested change

}

}

greptile-apps

Additional Comments (1)

src/vma/event/delta_timer.cpp, line 31-32 (link)

syntax: !_node_->handler doesn't compile - handler is now std::atomic<timer_handler*> and requires .load()

_{10 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/vma/event/timer_handler.h

greptile-apps

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}