Fix GError handling

Several users have reported segfaults when starting up profanity which
has OMEMO support, but OMEMO is not set up yet.

@StefanKropp has been able to reproduce this and tracked it down to
`_load_identity()` calling `omemo_known_devices_keyfile_save()`.
The latter then calls `save_keyfile()` which calls
`g_key_file_save_to_file()`. This can then fail if one of the first two
strings is NULL and won't set the `error` on return. In its error handling
`save_keyfile()` unconditionally dereferences `error` which leads to the
segfault.

Fix this and also go through the entire codebase and verify that the usage
of `GError` is done correctly.

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

.github/workflows/main.yml

@@ -35,10 +35,10 @@ jobs:
       # if this check fails, you have to update the number of auto types known and the list of auto types in the check below
       - name: Check auto types are up-to-date
         run: |
-          [[ "$(find src -type f -name '*.[ch]' -exec awk '/^#define auto_[\W]*/ {print $2}' '{}' \; | sort -u | wc -l)" == "8" ]] || exit -1
+          [[ "$(find src -type f -name '*.[ch]' -exec awk '/^#define auto_[\W]*/ {print $2}' '{}' \; | sort -u | wc -l)" == "9" ]] || exit -1
       - name: Check auto types are initialized
         run: |
-          grep -P 'auto_(char|gchar|gcharv|guchar|jid|sqlite|gfd|FILE)[\w *]*;$' -r src && exit -1 || true
+          grep -P 'auto_(char|gchar|gcharv|gerror|guchar|jid|sqlite|gfd|FILE)[\w *]*;$' -r src && exit -1 || true
       - name: Run clang-format
         uses: jidicula/clang-format-action@v4.11.0
         with:

src/command/cmd_funcs.c

@@ -7040,20 +7040,16 @@ cmd_plugins(ProfWin* window, const char* const command, gchar** args)
     GDir* global_cp_dir = NULL;
 
     if (access(GLOBAL_PYTHON_PLUGINS_PATH, R_OK) == 0) {
-        GError* error = NULL;
+        global_pyp_dir = g_dir_open(GLOBAL_PYTHON_PLUGINS_PATH, 0, NULL);
-        global_pyp_dir = g_dir_open(GLOBAL_PYTHON_PLUGINS_PATH, 0, &error);
+        if (global_pyp_dir == NULL) {
-        if (error) {
             log_warning("Error when trying to open global plugins path: %s", GLOBAL_PYTHON_PLUGINS_PATH);
-            g_error_free(error);
             return TRUE;
         }
     }
     if (access(GLOBAL_C_PLUGINS_PATH, R_OK) == 0) {
-        GError* error = NULL;
+        global_cp_dir = g_dir_open(GLOBAL_C_PLUGINS_PATH, 0, NULL);
-        global_cp_dir = g_dir_open(GLOBAL_C_PLUGINS_PATH, 0, &error);
+        if (global_cp_dir == NULL) {
-        if (error) {
             log_warning("Error when trying to open global plugins path: %s", GLOBAL_C_PLUGINS_PATH);
-            g_error_free(error);
             return TRUE;
         }
     }

src/common.c

@@ -161,20 +161,27 @@ auto_close_FILE(FILE** fd)
         log_error(g_strerror(errno));
 }
 
+void
+auto_free_gerror(GError** err)
+{
+    if (err == NULL)
+        return;
+
+    PROF_GERROR_FREE(*err);
+}
+
 static gboolean
 _load_keyfile(prof_keyfile_t* keyfile)
 {
-    GError* error = NULL;
+    auto_gerror GError* error = NULL;
     keyfile->keyfile = g_key_file_new();
 
     if (g_key_file_load_from_file(keyfile->keyfile, keyfile->filename, G_KEY_FILE_KEEP_COMMENTS | G_KEY_FILE_KEEP_TRANSLATIONS, &error)) {
         return TRUE;
-    } else if (error->code != G_FILE_ERROR_NOENT) {
+    } else if (error && error->code != G_FILE_ERROR_NOENT) {
         log_warning("[Keyfile] error loading %s: %s", keyfile->filename, error->message);
-        g_error_free(error);
     } else {
         log_warning("[Keyfile] no such file: %s", keyfile->filename);
-        g_error_free(error);
     }
     return FALSE;
 }
@@ -212,10 +219,9 @@ load_custom_keyfile(prof_keyfile_t* keyfile, gchar* filename)
 gboolean
 save_keyfile(prof_keyfile_t* keyfile)
 {
-    GError* error = NULL;
+    auto_gerror GError* error = NULL;
     if (!g_key_file_save_to_file(keyfile->keyfile, keyfile->filename, &error)) {
-        log_error("[Keyfile]: saving file %s failed! %s", keyfile->filename, error->message);
+        log_error("[Keyfile]: saving file %s failed! %s", STR_MAYBE_NULL(keyfile->filename), PROF_GERROR_MESSAGE(error));
-        g_error_free(error);
         return FALSE;
     }
     g_chmod(keyfile->filename, S_IRUSR | S_IWUSR);
@@ -254,11 +260,8 @@ copy_file(const char* const sourcepath, const char* const targetpath, const gboo
 {
     GFile* source = g_file_new_for_path(sourcepath);
     GFile* dest = g_file_new_for_path(targetpath);
-    GError* error = NULL;
     GFileCopyFlags flags = overwrite_existing ? G_FILE_COPY_OVERWRITE : G_FILE_COPY_NONE;
-    gboolean success = g_file_copy(source, dest, flags, NULL, NULL, NULL, &error);
+    gboolean success = g_file_copy(source, dest, flags, NULL, NULL, NULL, NULL);
-    if (error != NULL)
-        g_error_free(error);
     g_object_unref(source);
     g_object_unref(dest);
     return success;
@@ -634,7 +637,7 @@ get_mentions(gboolean whole_word, gboolean case_sensitive, const char* const mes
 gboolean
 call_external(gchar** argv)
 {
-    GError* spawn_error;
+    auto_gerror GError* spawn_error = NULL;
     gboolean is_successful;
 
     GSpawnFlags flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
@@ -647,9 +650,7 @@ call_external(gchar** argv)
                                   &spawn_error);
     if (!is_successful) {
         auto_gchar gchar* cmd = g_strjoinv(" ", argv);
-        log_error("Spawning '%s' failed with error '%s'", cmd, spawn_error ? spawn_error->message : "Unknown, spawn_error is NULL");
+        log_error("Spawning '%s' failed with error '%s'", cmd, PROF_GERROR_MESSAGE(spawn_error));
-
-        g_error_free(spawn_error);
     }
 
     return is_successful;

src/common.h

@@ -112,12 +112,22 @@ void auto_close_gfd(gint* fd);
 
 void auto_close_FILE(FILE** fd);
 
+void auto_free_gerror(GError** err);
+#define auto_gerror __attribute__((__cleanup__(auto_free_gerror)))
+
 #if defined(__OpenBSD__)
 #define STR_MAYBE_NULL(p) (p) ?: "(null)"
 #else
 #define STR_MAYBE_NULL(p) (p)
 #endif
 
+#define PROF_GERROR_MESSAGE(err) (err) ? (err)->message : "error message missing"
+#define PROF_GERROR_FREE(err)  \
+    do {                       \
+        if (err)               \
+            g_error_free(err); \
+    } while (0)
+
 typedef struct prof_keyfile_t
 {
     gchar* filename;

src/config/cafile.c

@@ -70,10 +70,10 @@ cafile_add(const TLSCertificate* cert)
     auto_gchar gchar* contents = NULL;
     auto_gchar gchar* new_contents = NULL;
     gsize length;
-    GError* glib_error = NULL;
+    auto_gerror GError* glib_error = NULL;
     if (g_file_test(cafile, G_FILE_TEST_EXISTS)) {
         if (!g_file_get_contents(cafile, &contents, &length, &glib_error)) {
-            log_error("[CAfile] could not read from %s: %s", cafile, glib_error ? glib_error->message : "No GLib error given");
+            log_error("[CAfile] could not read from %s: %s", cafile, PROF_GERROR_MESSAGE(glib_error));
             return;
         }
         if (strstr(contents, cert->fingerprint)) {
@@ -84,7 +84,7 @@ cafile_add(const TLSCertificate* cert)
     const char* header = "# Profanity CAfile\n# DO NOT EDIT - this file is automatically generated";
     new_contents = g_strdup_printf("%s\n\n# %s\n%s", contents ? contents : header, cert->fingerprint, cert->pem);
     if (!g_file_set_contents(cafile, new_contents, -1, &glib_error))
-        log_error("[CAfile] could not write to %s: %s", cafile, glib_error ? glib_error->message : "No GLib error given");
+        log_error("[CAfile] could not write to %s: %s", cafile, PROF_GERROR_MESSAGE(glib_error));
 }
 
 gchar*

src/config/preferences.c

@@ -89,7 +89,7 @@ _prefs_load(void)
     log_maxsize = g_key_file_get_integer(prefs, PREF_GROUP_LOGGING, "maxsize", &err);
     if (err) {
         log_maxsize = 0;
-        g_error_free(err);
+        PROF_GERROR_FREE(err);
     }
 
     // move pre 0.5.0 autoaway.time to autoaway.awaytime

src/main.c

@@ -93,9 +93,9 @@ main(int argc, char** argv)
     context = g_option_context_new(NULL);
     g_option_context_add_main_entries(context, entries, NULL);
     if (!g_option_context_parse(context, &argc, &argv, &error)) {
-        g_print("%s\n", error->message);
+        g_print("%s\n", PROF_GERROR_MESSAGE(error));
         g_option_context_free(context);
-        g_error_free(error);
+        PROF_GERROR_FREE(error);
         return 1;
     }
 

src/omemo/omemo.c

@@ -1426,11 +1426,10 @@ omemo_automatic_start(const char* const recipient)
 static gboolean
 _load_identity(void)
 {
-    GError* error = NULL;
+    auto_gerror GError* error = NULL;
     log_info("[OMEMO] Loading OMEMO identity");
 
     /* Device ID */
-    error = NULL;
     omemo_ctx.device_id = g_key_file_get_uint64(omemo_ctx.identity.keyfile, OMEMO_STORE_GROUP_IDENTITY, OMEMO_STORE_KEY_DEVICE_ID, &error);
     if (error != NULL) {
         log_error("[OMEMO] cannot load device id: %s", error->message);
@@ -1439,7 +1438,6 @@ _load_identity(void)
     log_debug("[OMEMO] device id: %d", omemo_ctx.device_id);
 
     /* Registration ID */
-    error = NULL;
     omemo_ctx.registration_id = g_key_file_get_uint64(omemo_ctx.identity.keyfile, OMEMO_STORE_GROUP_IDENTITY, OMEMO_STORE_KEY_REGISTRATION_ID, &error);
     if (error != NULL) {
         log_error("[OMEMO] cannot load registration id: %s", error->message);
@@ -1447,10 +1445,9 @@ _load_identity(void)
     }
 
     /* Identity key */
-    error = NULL;
     auto_gchar gchar* identity_key_public_b64 = g_key_file_get_string(omemo_ctx.identity.keyfile, OMEMO_STORE_GROUP_IDENTITY, OMEMO_STORE_KEY_IDENTITY_KEY_PUBLIC, &error);
     if (!identity_key_public_b64) {
-        log_error("[OMEMO] cannot load identity public key: %s", error->message);
+        log_error("[OMEMO] cannot load identity public key: %s", PROF_GERROR_MESSAGE(error));
         return FALSE;
     }
 
@@ -1458,10 +1455,9 @@ _load_identity(void)
     auto_guchar guchar* identity_key_public = g_base64_decode(identity_key_public_b64, &identity_key_public_len);
     omemo_ctx.identity_key_store.public = signal_buffer_create(identity_key_public, identity_key_public_len);
 
-    error = NULL;
     auto_gchar gchar* identity_key_private_b64 = g_key_file_get_string(omemo_ctx.identity.keyfile, OMEMO_STORE_GROUP_IDENTITY, OMEMO_STORE_KEY_IDENTITY_KEY_PRIVATE, &error);
     if (!identity_key_private_b64) {
-        log_error("[OMEMO] cannot load identity private key: %s", error->message);
+        log_error("[OMEMO] cannot load identity private key: %s", PROF_GERROR_MESSAGE(error));
         return FALSE;
     }
 

src/pgp/gpg.c

@@ -186,7 +186,7 @@ p_gpg_on_connect(const char* const barejid)
         auto_gchar gchar* keyid = g_key_file_get_string(pubkeyfile, jid, "keyid", &gerr);
         if (gerr) {
             log_error("Error loading PGP key id for %s", jid);
-            g_error_free(gerr);
+            PROF_GERROR_FREE(gerr);
         } else {
             gpgme_key_t key = NULL;
             error = gpgme_get_key(ctx, keyid, &key, 0);

src/plugins/plugins.c

@@ -214,10 +214,8 @@ plugins_uninstall(const char* const plugin_name)
     g_string_append(target_path, "/");
     g_string_append(target_path, plugin_name);
     GFile* file = g_file_new_for_path(target_path->str);
-    GError* error = NULL;
+    gboolean result = g_file_delete(file, NULL, NULL);
-    gboolean result = g_file_delete(file, NULL, &error);
     g_object_unref(file);
-    g_error_free(error);
     g_string_free(target_path, TRUE);
     return result;
 }

src/tools/editor.c

@@ -55,7 +55,7 @@ get_message_from_editor(gchar* message, gchar** returned_message)
     *returned_message = NULL;
 
     auto_gchar gchar* filename = NULL;
-    GError* glib_error = NULL;
+    auto_gerror GError* glib_error = NULL;
     const char* jid = connection_get_barejid();
     if (jid) {
         filename = files_file_in_account_data_path(DIR_EDITOR, jid, "compose.md");
@@ -78,10 +78,7 @@ get_message_from_editor(gchar* message, gchar** returned_message)
     }
 
     if (!g_file_set_contents(filename, message, messagelen, &glib_error)) {
-        log_error("[Editor] could not write to %s: %s", filename, glib_error ? glib_error->message : "No GLib error given");
+        log_error("[Editor] could not write to %s: %s", filename, PROF_GERROR_MESSAGE(glib_error));
-        if (glib_error) {
-            g_error_free(glib_error);
-        }
         return TRUE;
     }
 
@@ -106,10 +103,7 @@ get_message_from_editor(gchar* message, gchar** returned_message)
         gchar* contents;
         gsize length;
         if (!g_file_get_contents(filename, &contents, &length, &glib_error)) {
-            log_error("[Editor] could not read from %s: %s", filename, glib_error ? glib_error->message : "No GLib error given");
+            log_error("[Editor] could not read from %s: %s", filename, PROF_GERROR_MESSAGE(glib_error));
-            if (glib_error) {
-                g_error_free(glib_error);
-            }
             return TRUE;
         }
         /* Remove all trailing new-line characters */

src/ui/notifier.c

@@ -97,14 +97,13 @@ _notify(const char* const message, int timeout, const char* const category)
         notify_notification_set_category(notification, category);
         notify_notification_set_urgency(notification, NOTIFY_URGENCY_NORMAL);
 
-        GError* error = NULL;
+        auto_gerror GError* error = NULL;
         gboolean notify_success = notify_notification_show(notification, &error);
 
         if (!notify_success) {
             log_error("Error sending desktop notification:");
             log_error("  -> Message : %s", message);
-            log_error("  -> Error   : %s", error->message);
+            log_error("  -> Error   : %s", PROF_GERROR_MESSAGE(error));
-            g_error_free(error);
         } else {
             log_debug("Notification sent.");
         }

src/ui/tray.c

@@ -81,7 +81,7 @@ _get_icons(void)
 
     auto_gchar gchar* icons_dir_s = files_get_config_path(DIR_ICONS);
     icons_dir = g_string_new(icons_dir_s);
-    GError* err = NULL;
+    auto_gerror GError* err = NULL;
 
     if (!g_file_test(icons_dir->str, G_FILE_TEST_IS_DIR)) {
         return;
@@ -109,8 +109,7 @@ _get_icons(void)
         }
         g_string_free(name, TRUE);
     } else {
-        log_error("Unable to open dir: %s", err->message);
+        log_error("Unable to open dir: %s", PROF_GERROR_MESSAGE(err));
-        g_error_free(err);
     }
     g_dir_close(dir);
     g_string_free(icons_dir, TRUE);

src/xmpp/avatar.c

@@ -110,11 +110,11 @@ avatar_set(const char* path)
 {
     auto_char char* expanded_path = get_expanded_path(path);
 
-    GError* err = NULL;
+    auto_gerror GError* err = NULL;
     GdkPixbuf* pixbuf = gdk_pixbuf_new_from_file(expanded_path, &err);
 
     if (pixbuf == NULL) {
-        cons_show_error("An error occurred while opening %s: %s.", expanded_path, err ? err->message : "No error message given");
+        cons_show_error("An error occurred while opening %s: %s.", expanded_path, PROF_GERROR_MESSAGE(err));
         return FALSE;
     }
 
@@ -138,7 +138,7 @@ avatar_set(const char* path)
     gsize len = -1;
 
     if (!gdk_pixbuf_save_to_buffer(pixbuf, &img_data, &len, "png", &err, NULL)) {
-        cons_show_error("Unable to scale and convert avatar.");
+        cons_show_error("Unable to scale and convert avatar: %s.", PROF_GERROR_MESSAGE(err));
         return FALSE;
     }
 
@@ -342,11 +342,10 @@ _avatar_request_item_result_handler(xmpp_stanza_t* const stanza, void* const use
         g_string_append(filename, ".webp");
     }
 
-    GError* err = NULL;
+    auto_gerror GError* err = NULL;
     if (g_file_set_contents(filename->str, de, size, &err) == FALSE) {
         log_error("Unable to save picture: %s", err->message);
         cons_show("Unable to save picture %s", err->message);
-        g_error_free(err);
     } else {
         cons_show("Avatar saved as %s", filename->str);
     }

src/xmpp/vcard.c

@@ -1338,11 +1338,10 @@ _vcard_photo_result(xmpp_stanza_t* const stanza, void* userdata)
         g_string_append(filename, ".webp");
     }
 
-    GError* err = NULL;
+    auto_gerror GError* err = NULL;
 
     if (g_file_set_contents(filename->str, (gchar*)photo->data, photo->length, &err) == FALSE) {
-        cons_show_error("Unable to save photo: %s", err->message);
+        cons_show_error("Unable to save photo: %s", PROF_GERROR_MESSAGE(err));
-        g_error_free(err);
         g_string_free(filename, TRUE);
         return 1;
     } else {
@@ -1360,7 +1359,7 @@ _vcard_photo_result(xmpp_stanza_t* const stanza, void* userdata)
         g_string_append(filename, "\"");
         auto_char char* cmd = str_replace(cmdtemplate, "%p", filename->str);
 
-        if (g_shell_parse_argv(cmd, &argc, &argv, &err) == FALSE) {
+        if (g_shell_parse_argv(cmd, &argc, &argv, NULL) == FALSE) {
             cons_show_error("Failed to parse command template");
         } else {
             if (!call_external(argv)) {