From 4f8e26db8f645e8d50a9a9b31e5935c6967de55d Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Tue, 30 Jun 2026 13:30:28 +0300 Subject: Implement the timer/garbage-collector (was disabled/unfinished) wrap.cpp had '//TIMR->run(); // TODO' but timr::run() was never defined (wouldn't compile if uncommented), and the intended timer thread (timr::start) ran in a pthread and would race with the single-threaded libevent main loop (no locking on the shared room/user maps). Net effect: idle timeouts never ran, ghost users lingered in the online list forever after disconnect, and message timestamps were always 00:00:00 (s_time was only updated by the never-run timer). Also, even single-threaded, check_timeout was unsafe: user::set_online(false) does p_room->del_elem during hashmap::run_func's live begin/end iteration -> iterator invalidation/crash. Fix: - hashmap::run_func (both overloads): snapshot the values into a vector before invoking the callback, so callbacks may safely delete from the map during iteration. - timr::tick(): single-threaded 1s-cadence tick (called from the libevent main loop) that updates s_time/s_uptime every tick, and at the top of each minute runs check_timeout (idle timeout + auto-away), posts a PING keepalive to all streams, every 10 min cleans the ip cache, hourly runs garbage collection. - main.cpp: register a periodic libevent timer (1s, EV_PERSIST) calling timr::tick() in the event loop - no new thread. B1 use-after-free (found by independent review, fixed): when an idle user is reaped it moves to garbage while its long-lived stream context is still alive; the hourly remove_garbage would delete the user, leaving the context's p_user dangling (UAF on later disconnect). Fixed by: - gcol::remove_garbage now keeps garbage users whose stream is still open (get_stream_fd() >= 0) until the stream closes, deleting only closed-stream users (collected via a new collect_users_ helper since the hash_map base is privately inherited and not directly iterable). - handle_stream_read now reaps the disconnecting user from its room (set_online(false), a no-op if already idle-reaped) so a clean disconnect immediately clears the online list instead of lingering until idle-reap. Also removed the debug 'cout << SETTING OFFLINE' spam in set_online. Verified: timestamps now real (e.g. 10:29:20 vs old 00:00:00); idle users reaped at the next top-of-minute tick (no crash); clean disconnect removes the user from the online list; idle-reaped-then-stream-closed does not UAF (0 restarts); normal multi-user chat still works. Two independent fresh-context reviews: APPROVE-WITH-NITS; the B1 UAF and disconnect-reap findings were addressed; the pre-existing flush_stream hard-error UAF edge was filed as a separate follow-up. --- ychat/src/chat/gcol.cpp | 29 +++++++++++++++++++++++++++-- ychat/src/chat/gcol.h | 1 + ychat/src/chat/user.cpp | 1 - ychat/src/main.cpp | 20 ++++++++++++++++++++ ychat/src/maps/hashmap.tmpl | 30 ++++++++++++++++++++++++------ ychat/src/sock/sock.cpp | 6 ++++++ ychat/src/time/timr.cpp | 43 +++++++++++++++++++++++++++++++++++++++++++ ychat/src/time/timr.h | 1 + 8 files changed, 122 insertions(+), 9 deletions(-) diff --git a/ychat/src/chat/gcol.cpp b/ychat/src/chat/gcol.cpp index dddb01d..e62c8d5 100644 --- a/ychat/src/chat/gcol.cpp +++ b/ychat/src/chat/gcol.cpp @@ -78,9 +78,28 @@ gcol::remove_garbage() } vec_rooms.clear(); - - p_map_users->run_func( delete_users_ ); + // Delete only users whose stream connection is already closed. A user may + // have been reaped (set_online(false)) while its long-lived stream frame + // was still open; deleting the user now would leave the stream context's + // p_user dangling (use-after-free on the later disconnect). Such users are + // kept until their stream closes (handle_stream_read clears the fd), then + // deleted on a later pass. Collect via run_func (the hash_map base is not + // directly iterable from here). + vector vec_all; + p_map_users->run_func( collect_users_, (void*) &vec_all ); + + vector vec_keep; + for ( vector::iterator it = vec_all.begin(); it != vec_all.end(); ++it ) + { + user* u = *it; + if ( u->get_stream_fd() >= 0 ) + vec_keep.push_back(u); + else + delete_users_( u ); // clean() + delete (matches the old behaviour) + } p_map_users->clear(); + for ( vector::iterator it = vec_keep.begin(); it != vec_keep.end(); ++it ) + p_map_users->add_elem( *it, tool::to_lower((*it)->get_name()) ); return true; } @@ -135,6 +154,12 @@ gcol::delete_users_( user *user_obj ) delete user_obj; } +void +gcol::collect_users_( user *user_obj, void *v_arg ) +{ + static_cast*>(v_arg)->push_back( user_obj ); +} + void gcol::lock_mutex() {} diff --git a/ychat/src/chat/gcol.h b/ychat/src/chat/gcol.h index 710fc5f..dde00fe 100644 --- a/ychat/src/chat/gcol.h +++ b/ychat/src/chat/gcol.h @@ -44,6 +44,7 @@ private: static void delete_users_( user* user_obj ); + static void collect_users_( user* user_obj, void* v_arg ); public: gcol(); diff --git a/ychat/src/chat/user.cpp b/ychat/src/chat/user.cpp index 5a35371..98f1dbc 100644 --- a/ychat/src/chat/user.cpp +++ b/ychat/src/chat/user.cpp @@ -185,7 +185,6 @@ user::set_online( bool b_online ) this -> b_online = b_online; if (!b_online) { - cout << "SETTING OFFLINE" << endl; // remove the user from its room. string s_user(get_name()); string s_user_lowercase(get_lowercase_name()); diff --git a/ychat/src/main.cpp b/ychat/src/main.cpp index 2033249..3552be1 100644 --- a/ychat/src/main.cpp +++ b/ychat/src/main.cpp @@ -86,6 +86,14 @@ parse_argc( int argc, char* argv[] ) return start_params; } +// Single-threaded periodic timer callback (1s) driving the chat timer work. +// Defined here so the event can be registered in main() before event_dispatch. +static void +timer_cb( int /*fd*/, short /*event*/, void* /*arg*/ ) +{ + wrap::TIMR->tick(); +} + int main(int argc, char* argv[]) { @@ -109,6 +117,18 @@ main(int argc, char* argv[]) event_init(); sign::init_event_handlers(); sock::init_event_handlers(); + + // Periodic timer in the libevent main loop (single-threaded, no racy + // pthread): drives time-of-day updates, idle timeouts/auto-away, the + // stream PING keepalive, ip-cache cleanup and garbage collection. See + // timr::tick(). + static struct event ev_timer; + event_set( &ev_timer, -1, EV_PERSIST, timer_cb, NULL ); + struct timeval tv; + tv.tv_sec = 1; + tv.tv_usec = 0; + event_add( &ev_timer, &tv ); + //wrap::SOCK->start(); event_dispatch(); diff --git a/ychat/src/maps/hashmap.tmpl b/ychat/src/maps/hashmap.tmpl index cea2131..383baf6 100644 --- a/ychat/src/maps/hashmap.tmpl +++ b/ychat/src/maps/hashmap.tmpl @@ -23,6 +23,8 @@ *: Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *:*/ +#include + template bool compare_allocator::operator()(key_type_ t_key_1, key_type_ t_key_2) const @@ -136,16 +138,32 @@ template void hashmap::run_func( void (*func)(obj_type) ) { - typename hashmap::iterator iter; - for ( iter = this->begin(); iter != this->end(); ++iter ) - ( *func ) ( iter->second ); + // Snapshot the values first so a callback may safely mutate the map while + // we iterate (e.g. check_timeout deletes a timed-out user from its room + // mid-iteration, which would invalidate a live begin/end iterator). + std::vector v_snap; + v_snap.reserve( this->size() ); + for ( typename hashmap::iterator iter = this->begin(); + iter != this->end(); ++iter ) + v_snap.push_back( iter->second ); + + for ( typename std::vector::iterator it = v_snap.begin(); + it != v_snap.end(); ++it ) + ( *func )( *it ); } template void hashmap::run_func( void (*func)(obj_type, void*), void* v_arg ) { - typename hashmap::iterator iter; - for ( iter = this->begin(); iter != this->end(); ++iter ) - ( *func ) ( iter->second, v_arg ); + // See above: snapshot so callbacks may mutate the map during iteration. + std::vector v_snap; + v_snap.reserve( this->size() ); + for ( typename hashmap::iterator iter = this->begin(); + iter != this->end(); ++iter ) + v_snap.push_back( iter->second ); + + for ( typename std::vector::iterator it = v_snap.begin(); + it != v_snap.end(); ++it ) + ( *func )( *it, v_arg ); } diff --git a/ychat/src/sock/sock.cpp b/ychat/src/sock/sock.cpp index 5fbcc93..b145f6b 100644 --- a/ychat/src/sock/sock.cpp +++ b/ychat/src/sock/sock.cpp @@ -597,7 +597,13 @@ sock::handle_stream_read(int i_fd, short event, void *p_arg) if ( p_context->p_event ) event_del( p_context->p_event ); if ( p_context->p_user ) + { + // Reap the user from its room (no-op if already offline, e.g. idle-reaped). + // Without this a cleanly-disconnected online user would linger in the + // room/online list until the next idle-timeout sweep. + p_context->p_user->set_online(false); p_context->p_user->clear_stream(); + } delete p_context; } diff --git a/ychat/src/time/timr.cpp b/ychat/src/time/timr.cpp index 0d049c3..ace4f49 100644 --- a/ychat/src/time/timr.cpp +++ b/ychat/src/time/timr.cpp @@ -128,6 +128,49 @@ timr::start( void *v_ptr ) } } +// Single-threaded periodic tick, called from the libevent main loop (see +// main.cpp) instead of the old, never-finished racy timer thread +// (TIMR->run() was referenced but undefined; timr::start() ran in a pthread +// and raced with the event loop). One tick == one second. Per-minute work +// runs when the wall-clock second is 0, matching the old cadence. +void +timr::tick() +{ + static time_t clock_start = 0; + if ( clock_start == 0 ) + time( &clock_start ); + + time_t clock_now; + time( &clock_now ); + tm time_now = *localtime( &clock_now ); + + set_time( difftime( clock_now, clock_start ), + time_now.tm_sec, time_now.tm_min, time_now.tm_hour ); + + if ( time_now.tm_sec != 0 ) + return; // per-minute work only at the top of each minute + + // Idle-timeout / auto-away sweep. Safe now that hashmap::run_func + // snapshots the values before iterating (set_online(false) deletes the + // user from its room mid-iteration). + int* p_timeout_settings = new int[3]; + p_timeout_settings[0] = tool::string2int(wrap::CONF->get_elem("chat.idle.timeout")); + p_timeout_settings[1] = tool::string2int(wrap::CONF->get_elem("chat.idle.awaytimeout")); + p_timeout_settings[2] = tool::string2int(wrap::CONF->get_elem("chat.idle.autoawaytimeout")); + wrap::CHAT->check_timeout( p_timeout_settings ); + delete p_timeout_settings; + + // Keep stream frames alive and detect dead connections. + string s_ping = "\n"; + wrap::CHAT->msg_post( &s_ping ); + + if ( time_now.tm_min % 10 == 0 ) + wrap::SOCK->clean_ipcache(); + + if ( time_now.tm_min == 0 ) + wrap::GCOL->remove_garbage(); +} + void timr::set_time( double d_uptime, int i_cur_seconds, int i_cur_minutes, int i_cur_hours ) { diff --git a/ychat/src/time/timr.h b/ychat/src/time/timr.h index 69d411c..b9a138c 100644 --- a/ychat/src/time/timr.h +++ b/ychat/src/time/timr.h @@ -47,6 +47,7 @@ public: bool get_timer_active() const; void start( void *v_ptr ); + void tick(); // single-threaded periodic tick (called from the libevent loop) void set_time( double d_uptime, int i_cur_seconds, int i_cur_minutes, int i_cur_hours ); string add_zero_to_front( string s_time ); -- cgit v1.2.3