After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710367 - Crash in g_settings_backend_dispatch_signal()
Crash in g_settings_backend_dispatch_signal()
Status: RESOLVED FIXED
Product: dconf
Classification: Core
Component: gsettings backend
0.16.x
Other Linux
: Normal major
: ---
Assigned To: dconf-maint
dconf-maint
: 703098 711375 727641 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-10-17 09:19 UTC by Milan Bouchet-Valat
Modified: 2018-02-02 13:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsettingsbackend: a minor simplification (9.09 KB, patch)
2014-03-12 01:56 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GSettingsBackend: fix a nasty race condition (5.66 KB, patch)
2014-03-12 01:56 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Milan Bouchet-Valat 2013-10-17 09:19:03 UTC
This crash has been reported a few times and has ~400 occurrences in the Fedora crash database. It happens with glib2-2.36.3-3.fc19.x86_64 and dconf-0.16.0-2.fc19.x86_64 (at least).

https://bugzilla.redhat.com/show_bug.cgi?id=873412

Core was generated by `evolution'.
Program terminated with signal 11, Segmentation fault.

Thread 6 (Thread 0x7f436c0d4a00 (LWP 5322))

  • #0 g_value_dup_string
    at gvaluetypes.c line 1136
  • #1 g_settings_set_property
    at gsettings.c line 498
  • #2 object_set_property
    at gobject.c line 1357
  • #3 g_object_constructor
    at gobject.c line 1868
  • #4 g_object_newv
    at gobject.c line 1718
  • #5 g_object_new_valist
    at gobject.c line 1835
  • #6 g_object_new
    at gobject.c line 1550
  • #7 g_settings_new
    at gsettings.c line 869
  • #8 e_module_load
    at evolution-module-prefer-plain.c line 36
  • #9 module_load
    at e-module.c line 138
  • #10 g_type_module_use
    at gtypemodule.c line 256
  • #11 g_type_module_use_plugin
    at gtypemodule.c line 322
  • #12 type_data_ref_Wm
    at gtype.c line 1221
  • #13 g_type_class_ref
    at gtype.c line 2921
  • #14 extensible_load_extension
    at e-extensible.c line 91
  • #15 e_type_traverse
    at e-module.c line 362
  • #16 g_object_newv
    at gobject.c line 1746
  • #17 g_object_new
    at gobject.c line 1547
  • #18 e_mail_config_welcome_page_new
    at e-mail-config-welcome-page.c line 181
  • #19 mail_config_assistant_constructed
    at e-mail-config-assistant.c line 543
  • #20 startup_assistant_constructed
    at e-startup-assistant.c line 116
  • #21 g_object_newv
    at gobject.c line 1746
  • #22 g_object_new_valist
    at gobject.c line 1835
  • #23 g_object_new
    at gobject.c line 1550
  • #24 e_startup_assistant_new
    at e-startup-assistant.c line 238
  • #25 startup_wizard_new_assistant
    at evolution-startup-wizard.c line 101
  • #26 startup_wizard_run
    at evolution-startup-wizard.c line 186
  • #27 startup_wizard_load_accounts
    at evolution-startup-wizard.c line 287
  • #28 g_closure_invoke
    at gclosure.c line 777
  • #29 signal_emit_unlocked_R
    at gsignal.c line 3551
  • #30 g_signal_emit_valist
    at gsignal.c line 3300
  • #31 g_signal_emit
    at gsignal.c line 3356
  • #32 e_shell_event
    at e-shell.c line 1864
  • #33 main
    at main.c line 700

Thread 1 (Thread 0x7f435f3ed700 (LWP 5333))

  • #0 g_settings_backend_dispatch_signal
    at gsettingsbackend.c line 340
  • #1 g_settings_backend_path_changed
    at gsettingsbackend.c line 496
  • #2 dconf_engine_change_notify
    at dconfsettingsbackend.c line 233
  • #3 dconf_engine_watch_established
    at dconf-engine.c line 735
  • #4 dconf_engine_watch_established
    at dconf-engine.c line 712
  • #5 dconf_gdbus_method_call_done
    at dconf-gdbus-thread.c line 230
  • #6 g_simple_async_result_complete
    at gsimpleasyncresult.c line 775
  • #7 g_dbus_connection_call_done
    at gdbusconnection.c line 5339
  • #8 g_simple_async_result_complete
    at gsimpleasyncresult.c line 775
  • #9 complete_in_idle_cb
    at gsimpleasyncresult.c line 787
  • #10 g_main_dispatch
    at gmain.c line 2715
  • #11 g_main_context_dispatch
    at gmain.c line 3219
  • #12 g_main_context_iterate
    at gmain.c line 3290
  • #13 g_main_context_iteration
    at gmain.c line 3351
  • #14 dconf_gdbus_worker_thread
    at dconf-gdbus-thread.c line 81
  • #15 g_thread_proxy
    at gthread.c line 797
  • #16 start_thread
    at pthread_create.c line 308
  • #17 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 114

Comment 1 Allison Karlitskaya (desrt) 2013-10-17 21:01:13 UTC
I have a strong suspicion that this is a once-too-many-unref of a GSettings object...
Comment 2 Milan Bouchet-Valat 2013-10-18 12:42:16 UTC
Do you mean this is probably a bug in Evolution? All reports I could find occurred there indeed (this is not a proof but...).
Comment 3 Milan Crha 2014-01-17 16:23:03 UTC
*** Bug 711375 has been marked as a duplicate of this bug. ***
Comment 4 Milan Crha 2014-01-30 18:51:41 UTC
(In reply to comment #1)
> I have a strong suspicion that this is a once-too-many-unref of a GSettings
> object...

Thanks for the suggestion. I did walk-through evolution-data-server and evolution code and checked how GSettings objects returned from g_settings_new() are unreffed, and as far as i can tell, all are unreffed only once. I even found three places where they were not freed at all.

I saw few other bug reports recently, which were about memory corruption, thus maybe this is a consequence of it?
Comment 5 Allison Karlitskaya (desrt) 2014-02-25 14:31:31 UTC
Confirming this.  It has nothing to do with a GSettings object having a connected signal handler at dispose time _or_ with one too many unref.

It's a plain stupid race.

A signal comes in just as someone calls unref() on the last reference on a GSettings in the main thread.  The dispose process starts in the main thread and gets to the point where it has started to call weak notifies.

The GSettingsBackend lock is acquired in the thread which received the signal.  It increases the refcount on the GSettings object, but it is too late.  We release the lock.

Back in the main thread, the lock is acquired and despite the fact that we increased the reference count, we're already running the weak notify handlers.  We remove ourselves from the list.

Back in the signal thread, we access the list, assuming that we will still be alive (since we took a ref, right?).  Crash.
Comment 6 Allison Karlitskaya (desrt) 2014-03-12 01:56:11 UTC
Created attachment 271563 [details] [review]
gsettingsbackend: a minor simplification

Change the order of the arguments on the (internal) keys_changed callback in
GSettingsListenerVTable.

This means that all functions in the table now fit the following signature:

  void (* f) (GObject             *target,
              GSettingsBackend    *backend,
              const gchar         *name_or_path,
              gpointer             origin_tag,
              const gchar * const *names);

allowing the possibility of arguments ignored at the end.

This allows us to simplify our dispatch-to-thread code in GSettingsBackend,
making it a bit less generic.

So far, this should be a straight refactor.
Comment 7 Allison Karlitskaya (desrt) 2014-03-12 01:56:15 UTC
Created attachment 271564 [details] [review]
GSettingsBackend: fix a nasty race condition

In the event that a GSettings object is being destroyed just as a change
signal is being delivered, the destroying thread will race with the
dconf worker thread for acquiring the lock on the GSettingsBackend.

If the signalling thread gets there first then the destroying thread
will block on the lock.  The signalling thread adds a reference to the
GSettings object that is being destroyed and releases the lock.  The
idea is that this should prevent the GSettings object from being
destroyed and thus maintain its entry in the list.  Unfortunately, the
weak reference notify function is already running and as soon as we
release the lock, the list entry is removed.

The signalling thread crashes.

This bug is indicative of a serious problem encountered in many
situations where GObject instances are touched from multiple threads.
Ideally, we will move to a place where g_object_ref() is not called at
all on the GSettings object from the dconf worker thread and instead, a
dispatch will be done without holding a reference (similar to how
GAppInfoMonitor presently works).  This would also prevent the
unfortunate case of someone dropping what they assume to be the last
reference on a GSettings object, only to have an already-pending signal
delivered once they return to the mainloop, crashing their program.

Making this change for GSettings (with multiple instances per thread,
the possibility of multiple backends and each instance being interested
in different events) is going to be extremely non-trivial, so it's not a
change that makes sense at this point in the cycle.

For now, we can do a relatively small and isolated tweak so that we
never access the list except under a lock.  We still perform the bad
pattern of acquiring a ref in a foreign thread which means that we still
risk delivering a signal to a GSettings object that the user has assumed
is dead (unless they explicitly disconnect their signal handler).  This
is a problem that we already had, however.
Comment 8 Allison Karlitskaya (desrt) 2014-03-14 13:47:06 UTC
Attachment 271563 [details] pushed as 698970f - gsettingsbackend: a minor simplification
Attachment 271564 [details] pushed as 3f119b2 - GSettingsBackend: fix a nasty race condition

Pushing on the basis of positive testing in Ubuntu (where this is
already being carried as a vendor patch).
Comment 9 Allison Karlitskaya (desrt) 2014-04-05 08:41:27 UTC
*** Bug 727641 has been marked as a duplicate of this bug. ***
Comment 10 Philip Withnall 2018-02-02 13:53:07 UTC
*** Bug 703098 has been marked as a duplicate of this bug. ***