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 697595 - g_main_context_unref unlocks a mutex twice
g_main_context_unref unlocks a mutex twice
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
2.36.x
Other Windows
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-04-08 23:19 UTC by LRN
Modified: 2013-04-10 18:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed fix (5.53 KB, patch)
2013-04-08 23:21 UTC, LRN
none Details | Review
gmain: fix double-unlock in g_main_context_unref() (1.27 KB, patch)
2013-04-10 15:58 UTC, Dan Winship
committed Details | Review

Description LRN 2013-04-08 23:19:29 UTC
The following code:

/* test if adding a signal watch for different message types calls the
 * respective callbacks. */
GST_START_TEST (test_watch_with_custom_context)
{
  GMainContext *ctx;
  GSource *source;
  guint num_eos = 0;
  guint num_app = 0;
  guint id, id_idle;

  test_bus = gst_bus_new ();

  ctx = g_main_context_new ();
  main_loop = g_main_loop_new (ctx, FALSE);

  source = gst_bus_create_watch (test_bus);
  g_source_set_callback (source, (GSourceFunc) gst_bus_async_signal_func, NULL,
      NULL);
  id = g_source_attach (source, ctx);
  g_source_unref (source);
  fail_if (id == 0);

  g_signal_connect (test_bus, "message::eos", (GCallback) message_func_eos,
      &num_eos);
  g_signal_connect (test_bus, "message::application",
      (GCallback) message_func_app, &num_app);

  source = g_idle_source_new ();
  g_source_set_callback (source, (GSourceFunc) send_messages, NULL, NULL);
  id_idle = g_source_attach (source, ctx);
  g_source_unref (source);
  fail_if (id_idle == 0);

  while (g_main_context_pending (ctx))
    g_main_context_iteration (ctx, FALSE);

  fail_unless_equals_int (num_eos, 10);
  fail_unless_equals_int (num_app, 10);

  g_source_remove (id);
  g_source_remove (id_idle);
  g_main_loop_unref (main_loop);
  g_main_context_unref (ctx);

  gst_object_unref (test_bus);
}

crashes in g_main_context_unref (), because context->mutex is unlocked twice in a row:
g_source_iter_init() stores the context pointer.
g_source_iter_next() is then repeatedly called.
g_source_iter_next() calls a SOURCE_UNREF() macro, which uses the context pointer stored earlier.
SOURCE_UNREF() calls g_source_unref_internal() with that context pointer and have_lock==1.
g_source_unref_internal() doesn't lock the context (have_lock==1), potentially destroys the source, and, if finalization function is to be called, temporarily unlocks the context - at which point it crashes, because it did not actually held that mutex.
Comment 1 LRN 2013-04-08 23:21:46 UTC
Created attachment 240995 [details] [review]
Proposed fix

Note that simply adding locking to g_main_context_unref() does not work and leads to double lock instead (i.e. process hangs up).
Comment 2 Dan Winship 2013-04-10 15:58:37 UTC
Created attachment 241179 [details] [review]
gmain: fix double-unlock in g_main_context_unref()

When unreffing a context with sources still attached, it would end up
unlocking an already-unlocked context, causing crashes on platforms
that (unlike Linux) actually check for that.

====

> Note that simply adding locking to g_main_context_unref() does not work and
> leads to double lock instead (i.e. process hangs up).

It seems to work fine for me; I suspect you forgot to adjust the
g_source_destroy_internal() call inside the loop.

Does this patch work for you?
Comment 3 LRN 2013-04-10 16:48:45 UTC
*facepalm*
Yeah, your patch works. Thanks.
Comment 4 Colin Walters 2013-04-10 17:56:32 UTC
Review of attachment 241179 [details] [review]:

Go-go gadget patch!
Comment 5 Dan Winship 2013-04-10 18:20:46 UTC
Attachment 241179 [details] pushed as 0513c85 - gmain: fix double-unlock in g_main_context_unref()