Handle format errors when parsing YAML metadata

Based on an initial patch by Neil Mayhew, with modifications from
Matthias Klumpp.

Resolves #41

Signed-off-by: Matthias Klumpp <matthias@tenstral.net>

src/as-metadata.h

@@ -67,14 +67,16 @@ typedef enum {
 
 /**
  * AsMetadataError:
- * @AS_METADATA_ERROR_FAILED:			Generic failure
+ * @AS_METADATA_ERROR_FAILED:			Generic failure.
+ * @AS_METADATA_ERROR_PARSE:			Unable to parse the metadata file.
  * @AS_METADATA_ERROR_UNEXPECTED_FORMAT_KIND:	Expected upstream metadata but got distro metadata, or vice versa.
  * @AS_METADATA_ERROR_NO_COMPONENT:		We expected a component in the pool, but couldn't find one.
  *
  * A metadata processing error.
  **/
 typedef enum {
 	AS_METADATA_ERROR_FAILED,
+	AS_METADATA_ERROR_PARSE,
 	AS_METADATA_ERROR_UNEXPECTED_FORMAT_KIND,
 	AS_METADATA_ERROR_NO_COMPONENT,
 	/*< private >*/

src/as-yamldata.c

@@ -125,10 +125,10 @@ dep11_print_unknown (const gchar *root, const gchar *key)
 }
 
 /**
- * as_yamldata_free_node:
+ * as_yaml_free_node:
  */
 static gboolean
-as_yamldata_free_node (GNode *node, gpointer data)
+as_yaml_free_node (GNode *node, gpointer data)
 {
 	if (node->data != NULL)
 		g_free (node->data);
@@ -137,22 +137,29 @@ as_yamldata_free_node (GNode *node, gpointer data)
 }
 
 /**
- * dep11_yaml_process_layer:
+ * as_yaml_process_layer:
  *
  * Create GNode tree from DEP-11 YAML document
  */
 static void
-dep11_yaml_process_layer (yaml_parser_t *parser, GNode *data)
+as_yaml_process_layer (yaml_parser_t *parser, GNode *data, GError **error)
 {
 	GNode *last_leaf = data;
 	GNode *last_scalar;
 	yaml_event_t event;
 	gboolean parse = TRUE;
 	gboolean in_sequence = FALSE;
+	GError *tmp_error = NULL;
 	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_PARSE,
+					"Invalid DEP-11 file found. Could not parse YAML: %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) */
@@ -177,7 +184,13 @@ 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);
+
+				as_yaml_process_layer (parser, last_leaf, &tmp_error);
+				if (tmp_error != NULL) {
+					g_propagate_error (error, tmp_error);
+					parse = FALSE;
+				}
+
 				last_leaf = last_scalar;
 				storage ^= YAML_VAL; /* Flip VAR/VAL, without touching SEQ */
 				break;
@@ -1906,7 +1919,8 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
 	yaml_event_t event;
 	gboolean header = TRUE;
 	gboolean parse = TRUE;
-	GPtrArray *cpts = NULL;;
+	gboolean ret = TRUE;
+	g_autoptr(GPtrArray) cpts = NULL;
 	AsYAMLDataPrivate *priv = GET_PRIVATE (ydt);
 
 	/* we ignore empty data - usually happens if the file is broken, e.g. by disk corruption
@@ -1934,16 +1948,35 @@ 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_PARSE,
+					"Invalid DEP-11 file found. Could not parse YAML: %s", parser.problem);
+			ret = FALSE;
+			break;
+		}
+
 		if (event.type == YAML_DOCUMENT_START_EVENT) {
 			GNode *n;
 			gchar *key;
 			gchar *value;
 			AsComponent *cpt;
 			gboolean header_found = FALSE;
-			GNode *root = g_node_new (g_strdup (""));
-
-			dep11_yaml_process_layer (&parser, root);
+			GError *tmp_error = NULL;
+			g_autoptr(GNode) root = NULL;
+
+			root = g_node_new (g_strdup (""));
+			as_yaml_process_layer (&parser, root, &tmp_error);
+			if (tmp_error != NULL) {
+				/* stop immediately, since we found an error when parsing the document */
+				g_propagate_error (error, tmp_error);
+				g_free (root->data);
+				yaml_event_delete (&event);
+				ret = FALSE;
+				parse = FALSE;
+				break;
+			}
 
 			if (header) {
 				for (n = root->children; n != NULL; n = n->next) {
@@ -1953,6 +1986,7 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
 								AS_METADATA_ERROR,
 								AS_METADATA_ERROR_FAILED,
 								"Invalid DEP-11 file found: Header invalid");
+						ret = FALSE;
 						break;
 					}
 
@@ -2005,6 +2039,7 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
 				if (cpt == NULL) {
 					g_warning ("Parsing of YAML metadata failed: Could not read data for component.");
 					parse = FALSE;
+					ret = FALSE;
 				}
 
 				/* add found component to the results set */
@@ -2015,21 +2050,24 @@ as_yamldata_parse_distro_data (AsYAMLData *ydt, const gchar *data, GError **erro
 					G_IN_ORDER,
 					G_TRAVERSE_ALL,
 					-1,
-					as_yamldata_free_node,
+					as_yaml_free_node,
 					NULL);
-			g_node_destroy (root);
 		}
 
 		/* stop if end of stream is reached */
 		if (event.type == YAML_STREAM_END_EVENT)
 			parse = FALSE;
 
-		yaml_event_delete(&event);
+		yaml_event_delete (&event);
 	}
 
 	yaml_parser_delete (&parser);
 
-	return cpts;
+	/* return NULL on error, otherwise return the list of found components */
+	if (ret)
+		return g_ptr_array_ref (cpts);
+	else
+		return NULL;
 }
 
 /**

tests/test-yamldata.c

@@ -192,18 +192,31 @@ test_yamlwrite (void)
 	/* TODO: Actually test the resulting output */
 }
 
+/**
+ * as_yaml_test_read_data:
+ *
+ * Helper function to read a single component from YAML data.
+ */
 AsComponent*
-as_yaml_test_read_data (const gchar *data)
+as_yaml_test_read_data (const gchar *data, GError **error)
 {
 	AsComponent *cpt;
-	GError *error = 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);
+	if (error == NULL) {
+		g_autoptr(GError) local_error = NULL;
+
+		cpts = as_yamldata_parse_distro_data (ydt, data, &local_error);
+		g_assert_no_error (local_error);
+		g_assert_nonnull (cpts);
+	} else {
+		cpts = as_yamldata_parse_distro_data (ydt, data, error);
+		if (cpts == NULL)
+			return NULL;
+	}
 
 	cpt = AS_COMPONENT (g_ptr_array_index (cpts, 0));
 	return g_object_ref (cpt);
@@ -237,7 +250,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 (yamldata_icons_legacy, NULL);
 	g_assert_cmpstr (as_component_get_id (cpt), ==, "org.example.Test");
 
 	icons = as_component_get_icons (cpt);
@@ -253,7 +266,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 (yamldata_icons_current, NULL);
 	g_assert_cmpstr (as_component_get_id (cpt), ==, "org.example.Test");
 
 	icons = as_component_get_icons (cpt);
@@ -283,14 +296,34 @@ 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 (yamldata_languages, NULL);
 	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);
 }
 
+/**
+ * test_yaml_corrupt_data:
+ *
+ * Test reading of a broken YAML document.
+ */
+void
+test_yaml_corrupt_data (void)
+{
+	g_autoptr(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_PARSE);
+	g_assert_null (cpt);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -316,6 +349,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/CorruptData", test_yaml_corrupt_data);
 
 	ret = g_test_run ();
 	g_free (datadir);