From ff90674fd801dd369231a20c47ebef0d08402e9e Mon Sep 17 00:00:00 2001

From: William Roberts <william.c.roberts@intel.com>

Date: Tue, 12 Jan 2021 14:12:48 -0600

Subject: [PATCH 1/6] tabrmd-options: fix memory leak

The tabrmd_options_t structure is initialized with static char *

strings. These strings can be replaced by g_option_context_parse().

However, g_option_context_parse() replaces the string with allocated

memory and thus needs a call to g_free. Either one would need to keep

track if glib allocated the string and conditionally free it, or just

set all the strings to glib allocated strings. This patch takes the

approach of always allocating the option strings.

Fixes leaks like:

==2677142==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 9 byte(s) in 1 object(s) allocated from:

    #8 0x7fbd1acd5da1 in g_option_context_parse (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5eda1)

    #9 0x4cc438 in parse_opts /home/wcrobert/workspace/tpm2-abrmd/src/tabrmd-options.c:103:10

    #10 0x4c7ffe in main /home/wcrobert/workspace/tpm2-abrmd/src/tabrmd.c:41:10

    #11 0x7fbd1a8770b2 in __libc_start_main /build/glibc-ZN95T4/glibc-2.31/csu/../csu/libc-start.c:308:16

    #12 0x42004d in _start (/home/wcrobert/workspace/tpm2-abrmd/src/tpm2-abrmd+0x42004d)

Signed-off-by: William Roberts <william.c.roberts@intel.com>

---

 src/tabrmd-init.c    |  2 ++

 src/tabrmd-options.c | 53 +++++++++++++++++++++++++++++++++++++++-----

 src/tabrmd-options.h | 11 +++++----

 src/tabrmd.c         |  4 +++-

 4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/src/tabrmd-init.c b/src/tabrmd-init.c

index 2ad7539..58e0103 100644

--- a/src/tabrmd-init.c

+++ b/src/tabrmd-init.c

@@ -99,6 +99,8 @@ gmain_data_cleanup (gmain_data_t *data)

     if (data->loop != NULL) {

         main_loop_quit (data->loop);

     }

+

+    tabrmd_options_free(&data->options);

 }

 /*

  * This function initializes and configures all of the long-lived objects

diff --git a/src/tabrmd-options.c b/src/tabrmd-options.c

index 0dd7b87..22f249c 100644

--- a/src/tabrmd-options.c

+++ b/src/tabrmd-options.c

@@ -16,6 +16,12 @@

 #define G_OPTION_FLAG_NONE 0

 #endif

+#define SET_STR_IF_NULL(var, value) \

+    do { \

+        var = var == NULL ? g_strdup(value) : var; \

+        g_assert(var); \

+    } while(0)

+

 /*

  * This is a GOptionArgFunc callback invoked from the GOption processor from

  * the parse_opts function below. It will be called when the daemon is

@@ -36,6 +42,22 @@ show_version (const gchar  *option_name,

     g_print ("tpm2-abrmd version %s\n", VERSION);

     exit (0);

 }

+

+/**

+ * Frees internal memory associated with a tabrmd_options_t struct.

+ * @param opts

+ *  The options to free, note it doesn't free opts itself.

+ */

+void

+tabrmd_options_free(tabrmd_options_t *opts)

+{

+    g_assert(opts);

+

+    g_clear_pointer(&opts->dbus_name, g_free);

+    g_clear_pointer(&opts->prng_seed_file, g_free);

+    g_clear_pointer(&opts->tcti_conf, g_free);

+}

+

 /**

  * This function parses the parameter argument vector and populates the

  * parameter 'options' structure with data needed to configure the tabrmd.

@@ -51,7 +73,7 @@ parse_opts (gint argc,

             gchar *argv[],

             tabrmd_options_t *options)

 {

-    gchar *logger_name = "stdout";

+    gchar *logger_name = NULL;

     GOptionContext *ctx;

     GError *err = NULL;

     gboolean session_bus = FALSE;

@@ -105,33 +127,52 @@ parse_opts (gint argc,

         return FALSE;

     }

     g_option_context_free (ctx);

+

+    /*

+     * Set unset STRING options to defaults, we do this so we can free allocated

+     * string options with gfree, having a mix of const and allocated ptr's

+     * causes leaks

+     */

+    SET_STR_IF_NULL(options->dbus_name, TABRMD_DBUS_NAME_DEFAULT);

+    SET_STR_IF_NULL(options->prng_seed_file, TABRMD_ENTROPY_SRC_DEFAULT);

+    SET_STR_IF_NULL(options->tcti_conf, TABRMD_TCTI_CONF_DEFAULT);

+    SET_STR_IF_NULL(logger_name, "stdout");

+

     /* select the bus type, default to G_BUS_TYPE_SESSION */

     options->bus = session_bus ? G_BUS_TYPE_SESSION : G_BUS_TYPE_SYSTEM;

-    if (set_logger (logger_name) == -1) {

+    gint ret = set_logger (logger_name);

+    if (ret == -1) {

         g_critical ("Unknown logger: %s, try --help\n", logger_name);

-        return FALSE;

+        g_free(logger_name);

+        goto error;

     }

+    g_free(logger_name);

+

     if (options->max_connections < 1 ||

         options->max_connections > TABRMD_CONNECTION_MAX)

     {

         g_critical ("maximum number of connections must be between 1 "

                     "and %d", TABRMD_CONNECTION_MAX);

-        return FALSE;

+        goto error;

     }

     if (options->max_sessions < 1 ||

         options->max_sessions > TABRMD_SESSIONS_MAX_DEFAULT)

     {

         g_critical ("max-sessions must be between 1 and %d",

                     TABRMD_SESSIONS_MAX_DEFAULT);

-        return FALSE;

+        goto error;

     }

     if (options->max_transients < 1 ||

         options->max_transients > TABRMD_TRANSIENT_MAX)

     {

         g_critical ("max-trans-obj parameter must be between 1 and %d",

                     TABRMD_TRANSIENT_MAX);

-        return FALSE;

+        goto error;

     }

     g_warning ("tcti_conf after: \"%s\"", options->tcti_conf);

     return TRUE;

+

+error:

+    tabrmd_options_free(options);

+    return FALSE;

 }

diff --git a/src/tabrmd-options.h b/src/tabrmd-options.h

index 4994920..d6bcfe9 100644

--- a/src/tabrmd-options.h

+++ b/src/tabrmd-options.h

@@ -15,10 +15,10 @@

     .max_connections = TABRMD_CONNECTIONS_MAX_DEFAULT, \

     .max_transients = TABRMD_TRANSIENT_MAX_DEFAULT, \

     .max_sessions = TABRMD_SESSIONS_MAX_DEFAULT, \

-    .dbus_name = TABRMD_DBUS_NAME_DEFAULT, \

-    .prng_seed_file = TABRMD_ENTROPY_SRC_DEFAULT, \

+    .dbus_name = NULL, \

+    .prng_seed_file = NULL, \

     .allow_root = FALSE, \

-    .tcti_conf = TABRMD_TCTI_CONF_DEFAULT, \

+    .tcti_conf = NULL, \

 }

 typedef struct tabrmd_options {

@@ -28,7 +28,7 @@ typedef struct tabrmd_options {

     guint           max_transients;

     guint           max_sessions;

     gchar          *dbus_name;

-    const gchar    *prng_seed_file;

+    gchar          *prng_seed_file;

     gboolean        allow_root;

     gchar          *tcti_conf;

 } tabrmd_options_t;

@@ -38,4 +38,7 @@ parse_opts (gint argc,

             gchar *argv[],

             tabrmd_options_t *options);

+void

+tabrmd_options_free(tabrmd_options_t *opts);

+

 #endif /* TABRMD_OPTIONS_H */

diff --git a/src/tabrmd.c b/src/tabrmd.c

index 7c93e90..e015de3 100644

--- a/src/tabrmd.c

+++ b/src/tabrmd.c

@@ -43,7 +43,8 @@ main (int argc, char *argv[])

     }

     if (geteuid() == 0 && ! gmain_data.options.allow_root) {

         g_print ("Refusing to run as root. Pass --allow-root if you know what you are doing.\n");

-        return 1;

+        ret = 1;

+        goto out;

     }

     g_mutex_init (&gmain_data.init_mutex);

@@ -63,6 +64,7 @@ main (int argc, char *argv[])

     if (ret == 0 && gmain_data.ipc_disconnected) {

         ret = EX_IOERR;

     }

+out:

     gmain_data_cleanup (&gmain_data);

     return ret;

 }

-- 

2.34.3