GNOME Bugzilla – Bug 792753
g-c-c may crash if cups is disabled
Last modified: 2018-02-09 12:33:00 UTC
If cups service is disabled, g-c-c may segfault. How to reproduce: 0. Stop cups service: sudo systemctl stop cups 1. Open g-c-c 2. Search for "disp" and click on the Display panel item Result: g-c-c segfaults gdb backtrace: [New Thread 0x7fffb8ea3700 (LWP 24548)] Thread 1 "gnome-control-c" received signal SIGSEGV, Segmentation fault. 0x000055555560caa0 in connection_test_cb (source_object=0x555555dc3e20, result=0x7fffa8002c40, user_data=0x555556ae15b0) at ../../../../../../Main/Software/src/gnome/gnome-control-center/panels/printers/cc-printers-panel.c:1088 1088 priv->cups_status_check_id = (gdb) thread apply all bt full
+ Trace 238355
Thread 9 (Thread 0x7fffb8ea3700 (LWP 24548))
Thread 7 (Thread 0x7fffb96a4700 (LWP 24546))
Thread 5 (Thread 0x7fffbbfff700 (LWP 24304))
Thread 1 (Thread 0x7ffff7f37b00 (LWP 24147))
Created attachment 367326 [details] [review] printers: Make the cups connection test cancellable Currently gnome-control-center could crash whenever a connection test is interrupted by the disposal of the Printers panel. Searching in the g-c-c shell for any query that could match the Printers panel would instantiate the CcPrintersPanel class. Since we perform a connection test to the printing server as soon as this object is created, a fast disposal of the panel (by choosing another search result) would cause the whole application to crash.
Review of attachment 367326 [details] [review]: A few minor comments below. I think, however, there is another problem here: when the CcPanelList page changes (e.g. goes from the main page to the Devices page), it always activated the first panel. This is true as well when Control Center is initialized with a panel as a parameter, and we end up loading the Wi-Fi panel, then quickly switch to the other panel. This might be a separate bug, but it definitely needs to be fixed as well. It doesn't, however, invalidate the need to have a more robust Printers panel, and that's why I think this patch is still necessary and highly appreciated =) ::: panels/printers/cc-printers-panel.c @@ +1084,3 @@ gboolean success; PpCups *cups = PP_CUPS (source_object); + GError *error = NULL; Is there any reason to not use g_autptr(GError) here? @@ +1095,3 @@ + { + g_warning ("Could not test connection: %s", error->message); + } Doesn't need the curly braces. ::: panels/printers/pp-cups.c @@ +118,3 @@ + { + g_task_return_boolean (task, http != NULL); + } Doesn't need the curly braces
(In reply to Georges Basile Stavracas Neto from comment #2) > Review of attachment 367326 [details] [review] [review]: > > A few minor comments below. > > I think, however, there is another problem here: when the CcPanelList page > changes (e.g. goes from the main page to the Devices page), it always > activated the first panel. This is true as well when Control Center is > initialized with a panel as a parameter, and we end up loading the Wi-Fi > panel, then quickly switch to the other panel. > > This might be a separate bug, but it definitely needs to be fixed as well. > It doesn't, however, invalidate the need to have a more robust Printers > panel, and that's why I think this patch is still necessary and highly > appreciated =) > > ::: panels/printers/cc-printers-panel.c > @@ +1084,3 @@ > gboolean success; > PpCups *cups = PP_CUPS (source_object); > + GError *error = NULL; > > Is there any reason to not use g_autptr(GError) here? We haven't used it yet in the Printers panel. If Marek is cool with that, sure. > > @@ +1095,3 @@ > + { > + g_warning ("Could not test connection: %s", error->message); > + } > > Doesn't need the curly braces. same, we usually use braces pretty much everywhere in Printers.
(In reply to Felipe Borges from comment #3) > > > > @@ +1095,3 @@ > > + { > > + g_warning ("Could not test connection: %s", error->message); > > + } > > > > Doesn't need the curly braces. > > same, we usually use braces pretty much everywhere in Printers. Oh, that's fine then.
(In reply to Felipe Borges from comment #3) > (In reply to Georges Basile Stavracas Neto from comment #2) > > Review of attachment 367326 [details] [review] [review] [review]: > > > > Is there any reason to not use g_autptr(GError) here? > > We haven't used it yet in the Printers panel. If Marek is cool with that, > sure. Hi, yes, it is ok with me and thank you for the patch.
Created attachment 367718 [details] [review] printers: Make the cups connection test cancellable Currently gnome-control-center could crash whenever a connection test is interrupted by the disposal of the Printers panel. Searching in the g-c-c shell for any query that could match the Printers panel would instantiate the CcPrintersPanel class. Since we perform a connection test to the printing server as soon as this object is created, a fast disposal of the panel (by choosing another search result) would cause the whole application to crash.
(In reply to Marek Kašík from comment #5) > (In reply to Felipe Borges from comment #3) > > (In reply to Georges Basile Stavracas Neto from comment #2) > > > Review of attachment 367326 [details] [review] [review] [review] [review]: > > > > > > Is there any reason to not use g_autptr(GError) here? > > > > We haven't used it yet in the Printers panel. If Marek is cool with that, > > sure. > > Hi, yes, it is ok with me and thank you for the patch. Thanks. I will be using g_autoptr from now on for newly introduced objects.
Review of attachment 367718 [details] [review]: Thank you for the patch. There are just 2 things which need to change. ::: panels/printers/cc-printers-panel.c @@ +831,3 @@ widget = (GtkWidget*) gtk_builder_get_object (priv->builder, "main-vbox"); if (priv->num_dests == 0 && !priv->new_printer_name) + pp_cups_connection_test_async (g_object_ref (cups), NULL, set_current_page, widget); You have to change this because it does not apply now (s/widget/self/). @@ +1097,3 @@ + } + + g_error_free (error); You have to remove this because it crashes g-c-c.
Btw, I was not able to reproduce the original issue.
Created attachment 368178 [details] [review] printers: Make the cups connection test cancellable Currently gnome-control-center could crash whenever a connection test is interrupted by the disposal of the Printers panel. Searching in the g-c-c shell for any query that could match the Printers panel would instantiate the CcPrintersPanel class. Since we perform a connection test to the printing server as soon as this object is created, a fast disposal of the panel (by choosing another search result) would cause the whole application to crash.
Review of attachment 368178 [details] [review]: Thank you for the patch. Push it to relevant branches please.
Attachment 368178 [details] pushed as 2ff5cfd - printers: Make the cups connection test cancellable