Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle errors from the yaml parser #41

Closed
wants to merge 2 commits into from
+66 −18
Closed

Handle errors from the yaml parser #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b54c81
Handle errors from the yaml parser
neilmayhew May 10, 2016
fa27d3d
Add a unit test for handling corrupt yaml data
neilmayhew May 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 src/as-yamldata.c
View file
Open in desktop
@@ -137,7 +137,7 @@ as_yamldata_free_node (GNode *node, gpointer data)
* Create GNode tree from DEP-11 YAML document
*/
static void
dep11_yaml_process_layer (yaml_parser_t *parser, GNode *data)
dep11_yaml_process_layer (yaml_parser_t *parser, GNode *data, GError **error)
{
GNode *last_leaf = data;
GNode *last_scalar;
@@ -147,7 +147,13 @@ dep11_yaml_process_layer (yaml_parser_t *parser, GNode *data)
int storage = YAML_VAR; /* the first element must always be of type VAR */

while (parse) {
yaml_parser_parse (parser, &event);
if (!yaml_parser_parse (parser, &event)) {
g_set_error (error,
AS_METADATA_ERROR,
AS_METADATA_ERROR_FAILED,
"Invalid DEP-11 file found: YAML parsing error (%s)", parser->problem);
break;
}

/* Parse value either as a new leaf in the mapping
* or as a leaf value (one of them, in case it's a sequence) */
@@ -172,7 +178,9 @@ 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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (*error != NULL)
parse = FALSE;
last_leaf = last_scalar;
storage ^= YAML_VAL; /* Flip VAR/VAL, without touching SEQ */
break;
@@ -1839,7 +1847,14 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
yaml_parser_set_input_string (&parser, (unsigned char*) data, strlen (data));

while (parse) {
yaml_parser_parse (&parser, &event);
if (!yaml_parser_parse (&parser, &event)) {
g_set_error (error,
AS_METADATA_ERROR,
AS_METADATA_ERROR_FAILED,
"Invalid DEP-11 file found: YAML parsing error (%s)", parser.problem);
break;
}

if (event.type == YAML_DOCUMENT_START_EVENT) {
GNode *n;
gchar *key;
@@ -1848,9 +1863,9 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
gboolean header_found = FALSE;
GNode *root = g_node_new (g_strdup (""));

dep11_yaml_process_layer (&parser, root);
dep11_yaml_process_layer (&parser, root, error);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check for the error here and return NULL if an error is set (remember to issue a yaml_parser_delete (&parser); before that to not leak memory)


if (header) {
if (header && *error == NULL) {
for (n = root->children; n != NULL; n = n->next) {
if ((n->data == NULL) || (n->children == NULL)) {
parse = FALSE;
@@ -1925,8 +1940,8 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
g_node_destroy (root);
}

/* stop if end of stream is reached */
if (event.type == YAML_STREAM_END_EVENT)
/* stop if end of stream is reached or error was encountered */
if (event.type == YAML_STREAM_END_EVENT || *error != NULL)
parse = FALSE;

yaml_event_delete(&event);
53 tests/test-yamldata.c
View file
Open in desktop
@@ -177,20 +177,31 @@ test_yamlwrite (void)
}

AsComponent*
as_yaml_test_read_data (const gchar *data)
as_yaml_test_read_data (const gchar *data, GError **error)
{
AsComponent *cpt;
GError *error = NULL;
AsComponent *cpt = NULL;
g_autoptr(GPtrArray) cpts = NULL;
g_autoptr(AsYAMLData) ydt = NULL;

ydt = as_yamldata_new ();

cpts = as_yamldata_parse_distro_data (ydt, data, &error);
g_assert_no_error (error);
cpts = as_yamldata_parse_distro_data (ydt, data, error);

cpt = AS_COMPONENT (g_ptr_array_index (cpts, 0));
return g_object_ref (cpt);
if (cpts->len > 0) {
cpt = AS_COMPONENT (g_ptr_array_index (cpts, 0));
g_object_ref (cpt);
}
return cpt;
}

AsComponent*
as_yaml_test_read_data_ok (const gchar *data)
{
GError *error = NULL;
AsComponent *cpt = as_yaml_test_read_data (data, &error);
g_assert_no_error (error);
g_assert_nonnull (cpt);
return cpt;
}

void
@@ -221,7 +232,7 @@ test_yaml_read_icons (void)
ydt = as_yamldata_new ();

/* check the legacy icons */
cpt = as_yaml_test_read_data (yamldata_icons_legacy);
cpt = as_yaml_test_read_data_ok (yamldata_icons_legacy);
g_assert_cmpstr (as_component_get_id (cpt), ==, "org.example.Test");

icons = as_component_get_icons (cpt);
@@ -237,7 +248,7 @@ test_yaml_read_icons (void)

/* check the new style icons tag */
g_object_unref (cpt);
cpt = as_yaml_test_read_data (yamldata_icons_current);
cpt = as_yaml_test_read_data_ok (yamldata_icons_current);
g_assert_cmpstr (as_component_get_id (cpt), ==, "org.example.Test");

icons = as_component_get_icons (cpt);
@@ -267,14 +278,35 @@ test_yaml_read_languages (void)
" - locale: en_GB\n"
" percentage: 100\n";

cpt = as_yaml_test_read_data (yamldata_languages);
cpt = as_yaml_test_read_data_ok (yamldata_languages);
g_assert_cmpstr (as_component_get_id (cpt), ==, "org.example.Test");

g_assert_cmpint (as_component_get_language (cpt, "de_DE"), ==, 48);
g_assert_cmpint (as_component_get_language (cpt, "en_GB"), ==, 100);
g_assert_cmpint (as_component_get_language (cpt, "invalid_C"), ==, -1);
}

void
test_yaml_corrupt_data (void)
{
GError *error = NULL;
g_autoptr(AsComponent) cpt = NULL;
const gchar *yamldata_corrupt = "---\n"
"ID: org.example.Test\n"
"\007\n";

cpt = as_yaml_test_read_data (yamldata_corrupt, &error);

g_assert_error (error, AS_METADATA_ERROR, AS_METADATA_ERROR_FAILED);
g_assert_null (cpt);

if (error != NULL) {
g_assert_true (g_str_has_prefix (error->message, "Invalid DEP-11 file found: YAML parsing error"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be considered too brittle, as it's closely tied to the exact format of the error messages generated.

g_error_free (error);
error = NULL;
}
}

int
main (int argc, char **argv)
{
@@ -300,6 +332,7 @@ main (int argc, char **argv)
g_test_add_func ("/YAML/Write", test_yamlwrite);
g_test_add_func ("/YAML/Read/Icons", test_yaml_read_icons);
g_test_add_func ("/YAML/Read/Languages", test_yaml_read_languages);
g_test_add_func ("/YAML/Read/Corrupt", test_yaml_corrupt_data);

ret = g_test_run ();
g_free (datadir);