GNOME Bugzilla – Bug 761659
Crash invoking signal handler with array + length param
Last modified: 2016-02-09 07:17:00 UTC
When connected to a signal whose parameters include an array with separate length parameter, GJS crashes if the signal is ever emitted. Here's an example: const Gio = imports.gi.Gio; let app = new Gio.Application({ application_id: 'com.example.Application', flags: Gio.ApplicationFlags.HANDLES_OPEN, }); app.connect('open', function () {}); app.run(['foo', 'bar', 'baz']); The crash is an assertion failure in gjs_array_from_fixed_size_array(), which shouldn't be called on an array with length parameter. Indeed, the calling code at arg.cpp:2801 has this comment: "arrays with length are handled outside of this function." Here's a patch that adds tests for this case and fixes the problem in the signal marshaller. Note I added a few commented-out tests as well, since it's still not possible to _emit_ a signal with array + length param from within GJS, but this patch makes it possible to _handle_ such a signal emitted from C.
Created attachment 320563 [details] [review] gi: Marshal variable array-typed signal arguments It can happen that we need to marshal a closure parameter which is a GValue holding a pointer to an array, with the array length passed in as a separate parameter. For example, GApplication::open is such a signal. Previously, the pointer GValue would be sent to gjs_value_from_g_value_internal() which sends it on to gjs_value_from_g_argument(), which states in its code that arrays with explicit length parameters should be handled elsewhere. Normally they are indeed handled elsewhere (see e.g. gjs_invoke_c_function() in function.cpp) but in closure_marshal() they are not. This adds code to closure_marshal() that handles that case, also removing the length parameter from the arguments passed to the closure. It also adds some tests for this case (requires update to gobject-introspection) and a similar case that was until now untested, namely passing an array with length parameter out of an introspected function. Lastly, it adds some asserts to gjs_value_from_g_value_internal() and gjs_value_from_g_argument() which try to give a hint what's going wrong when you call those functions without having checked for this case.
Created attachment 320564 [details] [review] tests: Add regression test for signal w. array+len This is a regression test for marshalling callback arguments from signals with an array parameter and separate length parameter into closures in the introspected language.
Review of attachment 320563 [details] [review]: Looks mostly good - some comments below. ::: gi/value.cpp @@ +203,3 @@ + * before we invoke the closure. + */ + skip = g_new0(gboolean, argc); Could you use g_newa() here like elsewhere in the function to avoid hitting the memory allocator? @@ +212,3 @@ + if (signal_info && argc > 1) { + /* Start at argument 1, skip the instance parameter */ + for(i = 0; i < argc; i++) You could avoid the argc > 1 condition in the outer if block, as this won't run anyway. @@ +228,3 @@ + } + + GIArgInfo *arg_info; It looks like this is leaked when argc == 1
Review of attachment 320564 [details] [review]: Looks good.
Created attachment 320685 [details] [review] gi: Marshal variable array-typed signal arguments It can happen that we need to marshal a closure parameter which is a GValue holding a pointer to an array, with the array length passed in as a separate parameter. For example, GApplication::open is such a signal. Previously, the pointer GValue would be sent to gjs_value_from_g_value_internal() which sends it on to gjs_value_from_g_argument(), which states in its code that arrays with explicit length parameters should be handled elsewhere. Normally they are indeed handled elsewhere (see e.g. gjs_invoke_c_function() in function.cpp) but in closure_marshal() they are not. This adds code to closure_marshal() that handles that case, also removing the length parameter from the arguments passed to the closure. It also adds some tests for this case (requires update to gobject-introspection) and a similar case that was until now untested, namely passing an array with length parameter out of an introspected function. Lastly, it adds some asserts to gjs_value_from_g_value_internal() and gjs_value_from_g_argument() which try to give a hint what's going wrong when you call those functions without having checked for this case.
(In reply to Cosimo Cecchi from comment #3) > ::: gi/value.cpp > @@ +228,3 @@ > + } > + > + GIArgInfo *arg_info; > > It looks like this is leaked when argc == 1 Removing the argc > 1 condition fixed this leak without actually changing anything. Thanks for the review, here's the updated patch.
Comment on attachment 320564 [details] [review] tests: Add regression test for signal w. array+len Attachment 320564 [details] pushed as cd355e8 - tests: Add regression test for signal w. array+len
Review of attachment 320685 [details] [review]: Thanks, looks good.
Attachment 320685 [details] pushed as 67d7d2d - gi: Marshal variable array-typed signal arguments