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 763382 - Improper async handling
Improper async handling
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: color
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Richard Hughes
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2016-03-09 16:51 UTC by Matthias Clasen
Modified: 2016-03-15 23:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
adds cancellables (13.02 KB, patch)
2016-03-15 11:20 UTC, Richard Hughes
none Details | Review
adds cancellables (12.31 KB, patch)
2016-03-15 15:44 UTC, Richard Hughes
none Details | Review
no warning on cancel (7.91 KB, patch)
2016-03-15 15:44 UTC, Richard Hughes
none Details | Review
fixes crash (1.56 KB, patch)
2016-03-15 15:45 UTC, Richard Hughes
committed Details | Review
fixes crash #2 (1.91 KB, patch)
2016-03-15 15:45 UTC, Richard Hughes
committed Details | Review
adds cancellables (12.53 KB, patch)
2016-03-15 17:01 UTC, Richard Hughes
committed Details | Review
no warning on cancel (7.34 KB, patch)
2016-03-15 17:01 UTC, Richard Hughes
committed Details | Review

Description Matthias Clasen 2016-03-09 16:51:40 UTC
Reported as a crash here: https://bugzilla.redhat.com/show_bug.cgi?id=1288850


My take:


Just briefly looking at the code, 

gsd_color_manager_start calls gsd_color_state_start, which starts all sort of async stuff, while operating on a borrowed reference to the state object that was passed in from the color manager.

gsd_color_manager_stop does nothing to interrupt the async stuff that might be happening with the state object and finalize throws away the reference to the state object.

That seems like an obvious opportunity for the async machinery to operate on a dead state object. Not saying this is what is happening here, but...

A cancellable might be whats needed here.
Comment 1 Richard Hughes 2016-03-15 11:20:56 UTC
Created attachment 323967 [details] [review]
adds cancellables
Comment 2 Bastien Nocera 2016-03-15 11:44:07 UTC
Review of attachment 323967 [details] [review]:

The code after this will still be crashy. If the objects you're manipulating are finalized when you're receiving the async result, you can't access that object as user_data. For example:

Start the async:
        cd_client_find_profile_by_filename (profiles->priv->client,
                                            cd_icc_get_filename (icc),
                                            NULL,
                                            gcm_session_find_profile_by_filename_cb,
                                            profiles);

->finalize is called, the cancellable is cancelled, the object is freed.

static void
gcm_session_find_profile_by_filename_cb (GObject *object,
                                         GAsyncResult *res,
                                         gpointer user_data)
{
        GError *error = NULL;
        CdProfile *profile;
        CdClient *client = CD_CLIENT (object);
        GsdColorProfiles *profiles = GSD_COLOR_PROFILES (user_data);

Here the user_data might be a dangling pointer, you should only access it after checking that the async wasn't cancelled.

        profile = cd_client_find_profile_by_filename_finish (client, res, &error);
        if (profile == NULL) {
                g_warning ("%s", error->message);

Don't print an error message if the call was cancelled either. There are a bunch of such errors in the color plugin.

gcm_session_client_connect_cb() also pokes at memory that could be freed (and there's a few more elsewhere).

::: plugins/color/gsd-color-manager.c
@@ +87,1 @@
 gsd_color_manager_stop (GsdColorManager *manager)

_stop() is supposed to undo everything that was done in _start(). So both your gsd_color_*_start() calls should be cancelled here (which you're doing) and the ->cancellable unref'ed. You'd create a new one in _start().

::: plugins/color/gsd-color-profiles.c
@@ +93,3 @@
                           GError          **error)
 {
+        g_set_object (&profiles->priv->cancellable, cancellable);

I wouldn't take a reference here, simply borrow the existing one and pass the pointer.

@@ +255,3 @@
         profiles = GSD_COLOR_PROFILES (object);
 
+        g_clear_object (&profiles->priv->cancellable);

Which would remove the need to take care of the cancellable's lifecycle.

::: plugins/color/gsd-color-state.c
@@ +1377,3 @@
 {
         /* coldplug the list of screens */
+        g_set_object (&state->priv->cancellable, cancellable);

Ditto.
Comment 3 Richard Hughes 2016-03-15 15:44:37 UTC
Created attachment 324010 [details] [review]
adds cancellables
Comment 4 Richard Hughes 2016-03-15 15:44:56 UTC
Created attachment 324011 [details] [review]
no warning on cancel
Comment 5 Richard Hughes 2016-03-15 15:45:08 UTC
Created attachment 324012 [details] [review]
fixes crash
Comment 6 Richard Hughes 2016-03-15 15:45:39 UTC
Created attachment 324014 [details] [review]
fixes crash #2
Comment 7 Bastien Nocera 2016-03-15 16:04:18 UTC
Review of attachment 324010 [details] [review]:

::: plugins/color/gsd-color-profiles.c
@@ +94,3 @@
+        GsdColorProfilesPrivate *priv = profiles->priv;
+
+        /* use a fresh cancellable for each start->stop operation */

Maybe you should cancel the outstanding operations as well?

@@ +266,3 @@
         profiles = GSD_COLOR_PROFILES (object);
 
+        g_clear_object (&profiles->priv->cancellable);

You need to cancel the cancellable as well (so either call _stop() or add the one-liner here.

::: plugins/color/gsd-color-state.c
@@ +1378,3 @@
+        GsdColorStatePrivate *priv = state->priv;
+
+        /* use a fresh cancellable for each start->stop operation */

Ditto about cancelling before starting anew.

@@ +1445,3 @@
         state = GSD_COLOR_STATE (object);
 
+        g_clear_object (&state->priv->cancellable);

Same as for the other file, you're not cancelling on finalize().
Comment 8 Bastien Nocera 2016-03-15 16:05:08 UTC
Review of attachment 324011 [details] [review]:

Rest looks good.

::: plugins/color/gsd-color-state.c
@@ +1213,3 @@
         for (i = 0; outputs[i] != NULL; i++) {
                 /* get CdDevice for this output */
+                cd_client_find_device_by_property (priv->client,

Unrelated change here.
Comment 9 Bastien Nocera 2016-03-15 16:06:08 UTC
Review of attachment 324012 [details] [review]:

Looks good.
Comment 10 Bastien Nocera 2016-03-15 16:07:21 UTC
Review of attachment 324014 [details] [review]:

Yep. Might be worth filing a bug against gnome-rr to have the API changed though.
Comment 11 Richard Hughes 2016-03-15 17:01:31 UTC
Created attachment 324028 [details] [review]
adds cancellables
Comment 12 Richard Hughes 2016-03-15 17:01:50 UTC
Created attachment 324029 [details] [review]
no warning on cancel
Comment 13 Bastien Nocera 2016-03-15 23:41:47 UTC
And cherry-picked into gnome-3-18 as well.