Handle errors from the yaml parser #41
Conversation
|
This could be the reason why this change wasn't noticed so far: The XML parser ignored garbage after the root node was closed, and for the YAML parser we simply ignored all errors in the format... Bad thing! |
| @@ -172,7 +178,7 @@ dep11_yaml_process_layer (yaml_parser_t *parser, GNode *data) | |||
| last_scalar = last_leaf; | |||
| if (in_sequence) | |||
| last_leaf = g_node_append (last_leaf, g_node_new (g_strdup ("-"))); | |||
| dep11_yaml_process_layer (parser, last_leaf); | |||
| dep11_yaml_process_layer (parser, last_leaf, error); | |||
There was a problem hiding this comment.
You would need to check if error is NULL after that statement (and then return from the function), because otherwise in the (unlikely) event that the recursion emits multiple errors, we will get a crash when an attempt is made to set an error on a non-NULL GError object.
|
I added a few remarks to the patch, you basically would need to exit as early as possible to prevent GError being set twice. |
|
I think there are some pre-existing places where memory will be leaked due to the event not being deleted. See the breaks on lines 1861 & 1879 (old numbers) or 1876 & 1894 (new numbers). |
| g_assert_null (cpt); | ||
|
|
||
| if (error != NULL) { | ||
| g_assert_true (g_str_has_prefix (error->message, "Invalid DEP-11 file found: YAML parsing error")); |
There was a problem hiding this comment.
This could be considered too brittle, as it's closely tied to the exact format of the error messages generated.
Based on an initial patch by Neil Mayhew, with modifications from Matthias Klumpp. Resolves #41 Signed-off-by: Matthias Klumpp <matthias@tenstral.net>
|
I committed your patch with some modifications to make the GError handling more robust and GLib-ish, and make the parse function return NULL correctly in case of an error. Thank you for your patch and general help in debugging this! (Also, it's always awesome to have people provide testsuite testcases with the first patch!) |
|
Oh, and btw: I will do a new AppStream release tomorrow, which will include these changes. |
|
Thanks for accepting my change. I've not done much programming in the glib environment before, so I'm not surprised it needed some improvements. It's been a fun and interesting experience working on this, but I hope that everything is fixed now (and SRUs made) so I can get back to my other projects! |
|
The SRUs are not in my hands - that's up to Ubuntus stable release maintainers. The AppStream changes are in the queue for proposed-updates, and the appstream-glib changes are pending to be uploaded. |
This is a more complete treatment of the problem described in #35
This change is