GNOME Bugzilla – Bug 661951
_g_ir_parser_parse_file should always return a gerror if it returns NULL, causes segfaults otherwise
Last modified: 2015-02-07 17:03:01 UTC
When the namespace tag is not presetn, g-ir-parser segfaults instead of exiting gracefully with an error message.
Just noticed that there was another case where _g_ir_parser_parse_file would return NULL but not a gerror. Updated bug summary.
Created attachment 199163 [details] [review] Fixes sefaults caused by _g_ir_parser_parse_file This sets an error in case _g_ir_parser_parse_file fails and has to return NULL
Review of attachment 199163 [details] [review]: ::: girepository/girparser.c @@ +3561,3 @@ + return NULL; + } + That's wrong, g_file_get_contents() should *always* set an error if it returns FALSE. @@ +3571,3 @@ + G_MARKUP_ERROR_INVALID_CONTENT, + "Expected a namespace tag in the gir file"); + } And in this case, fix _g_ir_parser_parse_string() to do so too - don't patch all callers (even if there is just one). See the rules under: http://developer.gnome.org/glib/stable/glib-Error-Reporting.html
Created attachment 199337 [details] [review] error is set in _g_ir_parser_parse_string when it returns NULL
Created attachment 199340 [details] [review] error is set in _g_ir_parser_parse_string when it returns NULL
Review of attachment 199340 [details] [review]: Patch looks right. I generally prefer to see git-formatted patches though: https://live.gnome.org/GnomeLove/SubmittingPatches In this case it's helpful for someone in the future to say *why* you made this change. For example, "Noticed this when passing malformed .gir file to g-ir-scanner" or something.
Created attachment 199344 [details] [review] Prevents a segfault when _g_ir_parser_parse_string returns NULL, error was not set
Will do, I noticed it while using Vala to generate the .so/.gir, check #661952
Created attachment 207839 [details] [review] new patch Seems the patch has been commited, but it introduces a new issue: it should check if the error has not already been set, see attached patch. Currently, I get: GLib-WARNING **: GError set over the top of a previous GError or uninitialized memory. This indicates a bug in someone's code. You must ensure an error is NULL before it's set. The overwriting error message was: Expected namespace element in the gir file
Created attachment 207840 [details] [review] updated patch Hmm, I'd left a printf-debug statement in previous patch.
Review of attachment 207840 [details] [review]: Looks ok. I changed the conditional to be if (error && *error == NULL) since the rules for GError allow for error to be NULL.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]