GNOME Bugzilla – Bug 775679
arg: don't crash when asked to convert a null strv to an array
Last modified: 2017-04-26 17:44:26 UTC
We have a patch in Endless for this -- see https://github.com/endlessm/gjs/commit/4098edf99da8c3a6cb96126e2338db6f6f2ac205 and the comment in there for rationale.
A NULL strv is not an empty array, it's null. An empty strv is a valid pointer to NULL. What API triggers this?
If I understand correctly, it's code generated by gdbus-codegen that triggers this, so it's unlikely to just be a misuse of the API. Sam, do you have some time to create a minimal reproducer?
Created attachment 347110 [details] [review] regress: Add test for setting a NULL strv in a GValue This uses the Everything.test_null_strv_in_gvalue() added to Regress in a later patch
Created attachment 347112 [details] [review] tests: Call Everything.test_null_strv_in_gvalue() Call Everything.test_null_strv_in_gvalue() from regress
(In reply to Sam Spilsbury from comment #3) > Created attachment 347110 [details] [review] [review] > regress: Add test for setting a NULL strv in a GValue > > This uses the Everything.test_null_strv_in_gvalue() added to Regress in a > later patch Ugh, mixed these two up - This one is for gobject-introspection , the other is for gjs
Review of attachment 347110 [details] [review]: I think you forgot the actual test? These are just the generated documentation changes.
Review of attachment 347112 [details] [review]: This will need rebasing on master, since we've switched test frameworks. But it should be pretty trivial.
(In reply to Philip Chimento from comment #6) > Review of attachment 347110 [details] [review] [review]: > > I think you forgot the actual test? These are just the generated > documentation changes. Oops, sorry I missed this! I'll re-attach those patches now
Created attachment 349267 [details] [review] regress: Add test for setting a NULL strv in a GValue Adds additional missing files from commit
Created attachment 349268 [details] [review] tests: Call Everything.test_null_strv_in_gvalue Rebased
Review of attachment 349268 [details] [review]: ::: installed-tests/js/testEverythingBasic.js @@ +588,3 @@ + it('correctly converts a NULL strv in a GValue to an empty array', function() { + let v = Everything.test_null_strv_in_gvalue(); Renamed to "Regress"
Review of attachment 349267 [details] [review]: Seems fine to me, but I'm not a gobject-introspection reviewer. Cosimo, could you take a look please?
Review of attachment 349267 [details] [review]: I'm not a gobject-introspection maintainer, but this one looks good to me as well.
Rico, could you review the gobject-introspection patch, please?
Review of attachment 349268 [details] [review]: Sam, I just realized that this patch lost the actual fix, somewhere along the line...
Created attachment 350348 [details] [review] arg: Don't crash when asked to convert a null strv to a value
Created attachment 350349 [details] [review] arg: Don't crash when asked to convert a null strv to a value
Review of attachment 350349 [details] [review]: +1. Would be accepted-commit-now, but we need the gobject-introspection patch to be merged. Rico, would you be able to take a look at that?
Comment on attachment 349267 [details] [review] regress: Add test for setting a NULL strv in a GValue Attachment 349267 [details] pushed as f9d58df - regress: Add test for setting a NULL strv in a GValue
Review of attachment 350349 [details] [review]: Thanks, Rico! I'll commit this later and bump the requirement to g-i 1.53.1 for the tests.