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 711064 - Adding child source to blocked source can cause a segfault
Adding child source to blocked source can cause a segfault
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
2.36.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-10-29 11:00 UTC by Ognyan Tonchev (redstar_)
Modified: 2013-11-03 23:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gmain: handle child sources with no context in unblock_source() (1.58 KB, patch)
2013-10-29 11:00 UTC, Ognyan Tonchev (redstar_)
none Details | Review
Don't releases the context mutex until child_source has been added to a context fromg_source_add_child_source() (2.40 KB, patch)
2013-10-29 15:46 UTC, Ognyan Tonchev (redstar_)
none Details | Review

Description Ognyan Tonchev (redstar_) 2013-10-29 11:00:05 UTC
Created attachment 258428 [details] [review]
gmain: handle child sources with no context in unblock_source()

g_source_add_child_source() releases the context lock before attaching child_source to context. And this causes trouble if parent source is blocked and g_main_dispatch() manages to lock the context mutex and call unblock_source() before g_source_attach().

Please advise if the attached patch looks ok. There is another way of solving it by calling g_source_attach_unlocked() from g_source_add_child_source() before releasing the context mutex.
Comment 1 Dan Winship 2013-10-29 14:03:58 UTC
(In reply to comment #0)
> Please advise if the attached patch looks ok. There is another way of solving
> it by calling g_source_attach_unlocked() from g_source_add_child_source()
> before releasing the context mutex.

That seems like it would be better.

You'll need to reorganize the code a bit though, because you want the g_wakeup_signal() call from g_source_attach() to get run. So you'd want to move that to g_source_attach_unlocked(), but it might be good to add a boolean flag to that saying whether or not to signal, since you *don't* want to do it when g_source_attach_unlocked() is recursively calling itself to attach its child sources (since you only want to signal once, for the parent, not for each child).
Comment 2 Ognyan Tonchev (redstar_) 2013-10-29 15:46:53 UTC
Created attachment 258450 [details] [review]
Don't releases the context mutex until child_source has been added to a context fromg_source_add_child_source() 

Here comes another patch
Comment 3 Dan Winship 2013-11-02 22:18:32 UTC
Comment on attachment 258450 [details] [review]
Don't releases the context mutex until child_source has been added to a context fromg_source_add_child_source() 

Hm... is there some reason you made it do the wakeup before attach the source rather than after like before? I guess it still works because the context is locked, but it seems a little confusing.

If your code works fine with the wakeup moved to the end, I'll just do that and then commit it.
Comment 4 Ognyan Tonchev (redstar_) 2013-11-03 13:17:11 UTC
Sorry, i have changed the behavior unintentionally, it still works for me if the wakeup is moved to the end.
Comment 5 Dan Winship 2013-11-03 23:12:58 UTC
committed. thanks