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