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 756134 - Segmentation fault on calling g_simple_action_group_add_action with bad action constructor call
Segmentation fault on calling g_simple_action_group_add_action with bad actio...
Status: RESOLVED NOTABUG
Product: glib
Classification: Platform
Component: gobject
2.44.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2015-10-06 17:48 UTC by Christian Stadelmann
Modified: 2015-11-02 12:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gapplication: reject actions without names (2.80 KB, patch)
2015-10-07 13:54 UTC, Lars Karlitski
none Details | Review
gapplication: reject actions without names (2.96 KB, patch)
2015-11-02 12:23 UTC, Lars Karlitski
committed Details | Review

Description Christian Stadelmann 2015-10-06 17:48:39 UTC
When calling Gio.SimpleAction() without an argument it will cause a SIGSEGV when added to a Gtk.ApplicationWindow. I know that this is a programming error of whoever does that. I think calling this constructor without an argument should not be possible in the first place. I don't know whether this is an issue in GObject, PyGObject or SimpleAction. Feel free to reassign since you probably know better than me.

Example code:


#!/usr/bin/python3

import sys

from gi.repository import Gtk, Gio

class Test(Gtk.Application):
	
	def __init__(self):
		Gtk.Application.__init__(self, application_id="foo.bar", \
			flags=Gio.ApplicationFlags.FLAGS_NONE)
		
		self.connect("activate", self.activateCb)
		
	def activateCb(self, application):
		window = Gtk.ApplicationWindow()
		quit_action = Gio.SimpleAction()
		window.add_action(quit_action)
		
if __name__ == '__main__':
	application = Test()
	application.run(sys.argv)


Example backtrace:

Thread #1 [python3] 13767 [core: 1] (Suspended : Signal : SIGSEGV:Segmentation fault)	
	g_str_hash() at ghash.c:1.871 0x7fffef71e630	
	g_hash_table_lookup_node() at ghash.c:375 0x7fffef71da63	
	g_hash_table_lookup() at ghash.c:1.147 0x7fffef71da63	
	g_simple_action_group_add_action() at gsimpleactiongroup.c:194 0x7fffef00812a	
	ffi_call_unix64() at unix64.S:76 0x7fffef4e2db0	
	ffi_call() at ffi64.c:525 0x7fffef4e2818	
	pygi_invoke_c_callable() at 0x7ffff00ff3f4	
	pygi_function_cache_invoke() at 0x7ffff01010d8	
	_callable_info_call() at 0x7ffff00f51d5	
	PyObject_Call() at abstract.c:2.067 0x7ffff79adf31
Comment 1 Matthias Clasen 2015-10-07 02:05:56 UTC
Just don't do it if it crashes - g_simple_action_new() takes arguments for a reason...
Comment 2 Christian Stadelmann 2015-10-07 08:26:11 UTC
I know that this is wrong. But shouldn't something (pygobject, gobject) prevent the user from doing so? Shouldn't it check that python code calling Gtk/Glib functions (including constructors) must respect the number and type of parameters?
Comment 3 Lars Karlitski 2015-10-07 13:54:34 UTC
Created attachment 312827 [details] [review]
gapplication: reject actions without names

> Just don't do it if it crashes - g_simple_action_new() takes arguments for a
> reason...

I disagree. This is python, not C.

Let's print a critical when creating a GSimpleAction without a name and when
trying to add an action without a name to GSimpleActionGroup (which is used by
GApplication).

The patch also contains a test case.
Comment 4 Emmanuele Bassi (:ebassi) 2015-10-07 15:03:06 UTC
Review of attachment 312827 [details] [review]:

::: gio/gsimpleactiongroup.c
@@ +193,2 @@
   action_name = g_action_get_name (action);
+  g_return_if_fail (action_name != NULL);

I would not use a g_return_if_fail() for a state consistency check, here, considering that it can easily be disabled — or, at least, more easily disabled than an assertion.

Either we use a critical warning that explains better what you have to do, e.g.:

  if (action_name == NULL)
    {
      g_critical ("The supplied action has no name. You must set the "
                  "GAction:name property when creating an action.");
      return;
    }

Or use a g_assert().
Comment 5 Lars Karlitski 2015-11-02 12:23:39 UTC
Created attachment 314629 [details] [review]
gapplication: reject actions without names

Good point, thanks. I've changed it to print the critical warning you
suggested.
Comment 6 Emmanuele Bassi (:ebassi) 2015-11-02 12:33:51 UTC
Review of attachment 314629 [details] [review]:

Looks good to me.
Comment 7 Lars Karlitski 2015-11-02 12:53:19 UTC
Comment on attachment 314629 [details] [review]
gapplication: reject actions without names

Attachment 314629 [details] pushed as ee718d3 - gapplication: reject actions without names