From 0c39ba49a21b8861d9ffb4bee546bd9927cf0b3c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Hr=C3=A1zk=C3=BD?= <lhrazky@redhat.com>
Date: Mon, 7 Oct 2019 16:33:48 +0200
Subject: [PATCH] Fix leaking log handlers in Sack (RhBug:1758737)

Stores the log handler ids in the sack and uses g_log_remove_handler()
in the sack destructor to remove the handlers.

The mechanism is a bit complex and is explained in a code comment.

https://bugzilla.redhat.com/show_bug.cgi?id=1758737
---
 python/hawkey/sack-py.cpp | 47 +++++++++++++++++++++++++++++++++------
 python/hawkey/sack-py.hpp |  1 -
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/python/hawkey/sack-py.cpp b/python/hawkey/sack-py.cpp
index e9253463..66479309 100644
--- a/python/hawkey/sack-py.cpp
+++ b/python/hawkey/sack-py.cpp
@@ -47,6 +47,22 @@ typedef struct {
     DnfSack *sack;
     PyObject *custom_package_class;
     PyObject *custom_package_val;
+
+    // g_log handler IDs
+    // Multiple sacks can be created during a run of an application and each
+    // sack opens a log file and registers two g_log handlers. To avoid dangling
+    // handlers with invalid FILE pointers (we close them when destroying the
+    // sack), we need to keep track of the handlers so that we can also remove
+    // them.
+    //
+    // g_log is clever about adding log handlers. It does store all handlers
+    // registered for a given domain, but only the one that was registered last
+    // is used. If you remove the last registered one, the next in line will be
+    // used. That means stacking sacks is ok, the handler from the last
+    // undeleted sack will be the one that is used.
+    guint default_log_handler_id;
+    guint libdnf_log_handler_id;
+
     FILE *log_out;
 } _SackObject;
 
@@ -121,8 +137,13 @@ sack_dealloc(_SackObject *o)
     Py_XDECREF(o->custom_package_val);
     if (o->sack)
         g_object_unref(o->sack);
-    if (o->log_out)
+
+    if (o->log_out) {
+        g_log_remove_handler(nullptr, o->default_log_handler_id);
+        g_log_remove_handler("libdnf", o->libdnf_log_handler_id);
         fclose(o->log_out);
+    }
+
     Py_TYPE(o)->tp_free(o);
 }
 
@@ -177,15 +198,27 @@ log_handler(const gchar *log_domain, GLogLevelFlags log_level, const gchar *mess
     g_free(msg);
 }
 
-gboolean
-set_logfile(const gchar *path, FILE *log_out)
+static void
+log_handler_noop(const gchar *, GLogLevelFlags, const gchar *, gpointer)
 {
-    log_out = fopen(path, "a");
+}
+
+static gboolean
+sack_set_logfile(_SackObject *self, const gchar *path)
+{
+    self->log_out = fopen(path, "a");
 
-    if (!log_out)
+    if (!self->log_out)
         return FALSE;
 
-    g_log_set_default_handler(log_handler, log_out);
+    // The default log handler prints messages that weren't handled by any
+    // other logger to stderr/stdout, we do not want that
+    g_log_set_default_handler(log_handler_noop, nullptr);
+
+    // set the handler for the default domain as well as "libdnf"
+    self->default_log_handler_id = g_log_set_handler(nullptr, G_LOG_LEVEL_MASK, log_handler, self->log_out);
+    self->libdnf_log_handler_id = g_log_set_handler("libdnf", G_LOG_LEVEL_MASK, log_handler, self->log_out);
+
     g_info("=== Started libdnf-%d.%d.%d ===", LIBDNF_MAJOR_VERSION,
             LIBDNF_MINOR_VERSION, LIBDNF_MICRO_VERSION);
     return TRUE;
@@ -237,7 +270,7 @@ sack_init(_SackObject *self, PyObject *args, PyObject *kwds)
         PycompString logfile(logfile_py);
         if (!logfile.getCString())
             return -1;
-        if (!set_logfile(logfile.getCString(), self->log_out)) {
+        if (!sack_set_logfile(self, logfile.getCString())) {
             PyErr_Format(PyExc_IOError, "Failed to open log file: %s", logfile.getCString());
             return -1;
         }
diff --git a/python/hawkey/sack-py.hpp b/python/hawkey/sack-py.hpp
index cba8accb..4ae77380 100644
--- a/python/hawkey/sack-py.hpp
+++ b/python/hawkey/sack-py.hpp
@@ -35,7 +35,6 @@ DnfSack *sackFromPyObject(PyObject *o);
 int sack_converter(PyObject *o, DnfSack **sack_ptr);
 
 PyObject *new_package(PyObject *sack, Id id);
-gboolean set_logfile(const gchar *path, FILE *log_out);
 const char *log_level_name(int level);
 
 #endif // SACK_PY_H
-- 
2.25.2