Clear out log handlers when disposing Debug to avoid crashers.

In cases where the IndividualAggregator is destroyed and instantiated
again (as in some of our tests), we could end up calling the Debug
log handlers after destroying the Debug instance (resulting in
g_static_rec_mutex_lock/unlock calls on an invalid freed mutex,
causing a segfault).

Closes: bgo#664158 - Stale log handler can cause segfault when
re-creating Aggregator in a single run

diff --git a/NEWS b/NEWS
index 39534ec..f2754c1 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,8 @@
 Overview of changes from libfolks 0.6.5 to libfolks 0.6.6
 =============================================================
 Bugs fixed:
+* Bug 664158 — Stale log handler can cause segfault when re-creating Aggregator
+  in a single run
 
 Overview of changes from libfolks 0.6.4.1 to libfolks 0.6.5
 =============================================================

diff --git a/folks/debug.vala b/folks/debug.vala
index 51f119d..cb8f652 100644 (file)
--- a/folks/debug.vala
+++ b/folks/debug.vala
@@ -57,6 +57,7 @@ public class Folks.Debug : Object
   private string _indentation_string = "";
 
   private bool _colour_enabled = true;
+  private HashSet<string> _domains_handled;
 
   /*
    * Whether colour output is enabled. If true, debug output may include
@@ -169,7 +170,7 @@ public class Folks.Debug : Object
         {
           if (this._all || this._domains.contains (domain.down ()))
             {
-              Log.set_handler (domain, LogLevelFlags.LEVEL_MASK,
+              this._set_handler (domain, LogLevelFlags.LEVEL_MASK,
                   this._log_handler_cb);
               return;
             }
@@ -177,7 +178,7 @@ public class Folks.Debug : Object
 
       /* Install a log handler which will blackhole the log message.
        * Other log messages will be printed out by the default log handler. */
-      Log.set_handler (domain, LogLevelFlags.LEVEL_DEBUG,
+      this._set_handler (domain, LogLevelFlags.LEVEL_DEBUG,
           (domain_arg, flags, message) => {});
     }
 
@@ -255,14 +256,21 @@ public class Folks.Debug : Object
     {
       /* Private constructor for singleton */
 
+      this._domains_handled = new HashSet<string> ();
+
       /* Install a log handler for log messages emitted as a result of
        * Debug.print-status being emitted. */
-      Log.set_handler (Debug.STATUS_LOG_DOMAIN, LogLevelFlags.LEVEL_MASK,
+      this._set_handler (Debug.STATUS_LOG_DOMAIN, LogLevelFlags.LEVEL_MASK,
           this._print_status_log_handler_cb);
     }
 
   ~Debug ()
     {
+      /* Remove handlers so they don't get called after we're destroyed */
+      foreach (var domain in this._domains_handled)
+        this._remove_handler (domain, true);
+      this._domains_handled.clear ();
+
       /* Manually clear the singleton _instance */
       lock (Debug._instance)
         {
@@ -270,6 +278,30 @@ public class Folks.Debug : Object
         }
     }
 
+  private void _set_handler (
+      string? domain,
+      LogLevelFlags flags,
+      LogFunc log_func)
+    {
+      this._remove_handler (domain);
+      Log.set_handler (domain, flags, log_func);
+      this._domains_handled.add (domain);
+    }
+
+  private void _remove_handler (string domain, bool keep_in_map = false)
+    {
+      if (this._domains_handled.contains (domain))
+        {
+          Log.set_handler (domain,
+              (LogLevelFlags.LEVEL_MASK | LogLevelFlags.FLAG_RECURSION |
+                  LogLevelFlags.FLAG_FATAL),
+              Log.default_handler);
+
+          if (!keep_in_map)
+            this._domains_handled.remove (domain);
+        }
+    }
+
   /**
    * Causes all significant objects in the library to print their current
    * status to standard output, obeying the options set on this