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.

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<user*> vec_all;
+  p_map_users->run_func( collect_users_, (void*) &vec_all );
+
+  vector<user*> vec_keep;
+  for ( vector<user*>::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<user*>::iterator it = vec_keep.begin(); it != vec_keep.end(); ++it )
+    p_map_users->add_elem( *it, tool::to_lower((*it)->get_name()) );
 
   return true;
 }
@@ -136,6 +155,12 @@ gcol::delete_users_( user *user_obj )
 }
 
 void
+gcol::collect_users_( user *user_obj, void *v_arg )
+{
+  static_cast<std::vector<user*>*>(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 <vector>
+
 template<class key_type_>
 bool
 compare_allocator<key_type_>::operator()(key_type_ t_key_1, key_type_ t_key_2) const
@@ -136,16 +138,32 @@ template<class obj_type, class key_type_, class hash_type, class alloc_type>
 void
 hashmap<obj_type, key_type_, hash_type, alloc_type>::run_func( void (*func)(obj_type) )
 {
-  typename hashmap<obj_type, key_type_, hash_type, alloc_type>::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<obj_type> v_snap;
+  v_snap.reserve( this->size() );
+  for ( typename hashmap<obj_type, key_type_, hash_type, alloc_type>::iterator iter = this->begin();
+        iter != this->end(); ++iter )
+    v_snap.push_back( iter->second );
+
+  for ( typename std::vector<obj_type>::iterator it = v_snap.begin();
+        it != v_snap.end(); ++it )
+    ( *func )( *it );
 }
 
 template<class obj_type, class key_type_, class hash_type, class alloc_type>
 void
 hashmap<obj_type, key_type_, hash_type, alloc_type>::run_func( void (*func)(obj_type, void*), void* v_arg )
 {
-  typename hashmap<obj_type, key_type_, hash_type, alloc_type>::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<obj_type> v_snap;
+  v_snap.reserve( this->size() );
+  for ( typename hashmap<obj_type, key_type_, hash_type, alloc_type>::iterator iter = this->begin();
+        iter != this->end(); ++iter )
+    v_snap.push_back( iter->second );
+
+  for ( typename std::vector<obj_type>::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 = "<!-- 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 );