mission-control-5 crashed with SIGABRT in __libc_message()

Created attachment 89967
reset the error to NULL, so the next call is working

That bug exists in 5.16 but doesn't apply to trunk due to some refactoring

Way to trigger it:
- create a user/.local/share/telepathy where the user has no write rights
- try running mission-control
- see the warnings and segfault

Stacktrace
#0 0xb77d1424 in __kernel_vsyscall ()
#1 0xb7039aff in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#2 0xb703d083 in __GI_abort () at abort.c:90
#3 0xb7076a95 in __libc_message (do_abort=do_abort@entry=2,
    fmt=fmt@entry=0xb717a0d0 "*** Error in `%s': %s: 0x%s ***\n")
    at ../sysdeps/unix/sysv/linux/libc_fatal.c:199
#4 0xb70817c2 in malloc_printerr (action=<optimized out>,
    str=<optimized out>, ptr=0x863b728) at malloc.c:4923
#5 0xb7082510 in _int_free (av=0xb71bb440 <main_arena>, p=0x863b720,
    have_lock=0) at malloc.c:3779
#6 0xb72265d0 in g_free (mem=mem@entry=0x863b728)
    at /build/buildd/glib2.0-2.38.1/./glib/gmem.c:197
#7 0x08085bdd in _commit (self=self@entry=0x85da000, am=am@entry=0x8638a78,
    account=account@entry=0x0) at mcd-account-manager-default.c:259
#8 0x08085ec0 in _list (self=0x85da000, am=0x8638a78)
    at mcd-account-manager-default.c:364
#9 0xb7797442 in mcp_account_storage_list (storage=storage@entry=0x85da000,
    am=am@entry=0x8638a78) at account-storage.c:952
#10 0x0808376e in mcd_storage_load (self=0x8638a78) at mcd-storage.c:740
#11 0x08070937 in _mcd_account_manager_constructed (obj=0x85d8680)
    at mcd-account-manager.c:1682

The code does g_error_free() an error to reuse it, but without resetting it to NULL, which makes the next call not set a new one and the g_warning hit an invalid read.

You should use g_clear_error() instead. I'm actually tempted to do a big s/g_error_free/g_clear_error/ and ban g_error_free() in our coding style checker, like we already banned some _free() in favor of their corresponding _unref().

(In reply to comment #1)
> You should use g_clear_error() instead

I agree...

> I'm actually tempted to do a big s/g_error_free/g_clear_error/

... but I think this is going too far - it's often obvious that the error is both non-NULL and no longer used, for instance in this idiom for dealing with errors that there's no way to signal (yes that's a design flaw, but often one that we're stuck with medium-term):

static void
foo (void)
{
  GError *error = NULL;

  if (do_a_thing (&error))
    {
      WARNING ("%s", error->message);
      g_error_free (error);
    }
}

or when interacting with dbus-glib:

static void
do_something_on_dbus (DBusGMethodInvocation *context)
{
  GError *error = NULL;

  if (do_something (&error))
    {
      dbus_g_method_return (context);
    }
  else
    {
      dbus_g_method_return_error (context, error);
      g_error_free (error);
    }
}

Created attachment 90260
new version using g_clear_error

ok, updated to use g_clear_error, I've only done that specific change and not tried to replace other uses

Comment on attachment 90260
new version using g_clear_error

Review of attachment 90260:
-----------------------------------------------------------------

Looks good to me.

Comment on attachment 90260
new version using g_clear_error

Review of attachment 90260:
-----------------------------------------------------------------

Actually, I take that back. This is g_clear_error (error), it needs to be g_clear_error (&error). Did you test this?

Fixed in git for 5.16.1; I added the necessary "&" to make it work.

ups, sorry about that, I test built but didn't test run in the buggy condition ... thanks for catching the error!