diff mbox series

parsers: Fix false positive parser format errors

Message ID 20240208162430.208984-1-michael.adler@siemens.com
State Accepted
Delegated to: Stefano Babic
Headers show
Series parsers: Fix false positive parser format errors | expand

Commit Message

Michael Adler Feb. 8, 2024, 4:24 p.m. UTC 
From: Oleksandr Makhmudov <oleksandr.makhmudov@siemens.com>

Parsers output errors to log when config format doesn't match the
parser. This causes false positive errors when config format is json
due to the libconfig parser being run first.

This can be fixed by caching these errors and only outputting them
if all parsers fail.

Signed-off-by: Oleksandr Makhmudov <oleksandr.makhmudov@siemens.com>
Signed-off-by: Michael Adler <michael.adler@siemens.com>
Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 core/parser.c           | 13 ++++++++++++-
 include/parsers.h       |  8 ++++----
 parser/parse_external.c |  8 +++++---
 parser/parser.c         | 21 ++++++++++++++-------
 4 files changed, 35 insertions(+), 15 deletions(-)

Comments

Stefano Babic Feb. 29, 2024, 8:47 a.m. UTC | #1 
On 08.02.24 17:24, 'Michael Adler' via swupdate wrote:
> From: Oleksandr Makhmudov <oleksandr.makhmudov@siemens.com>
>
> Parsers output errors to log when config format doesn't match the
> parser. This causes false positive errors when config format is json
> due to the libconfig parser being run first.
>
> This can be fixed by caching these errors and only outputting them
> if all parsers fail.
>
> Signed-off-by: Oleksandr Makhmudov <oleksandr.makhmudov@siemens.com>
> Signed-off-by: Michael Adler <michael.adler@siemens.com>
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   core/parser.c           | 13 ++++++++++++-
>   include/parsers.h       |  8 ++++----
>   parser/parse_external.c |  8 +++++---
>   parser/parser.c         | 21 ++++++++++++++-------
>   4 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/core/parser.c b/core/parser.c
> index 02fb38a7..5cd611fc 100644
> --- a/core/parser.c
> +++ b/core/parser.c
> @@ -151,20 +151,31 @@ int parse(struct swupdate_cfg *sw, const char *descfile)
>   		return ret;
>
>   #endif
> +	char *errors[ARRAY_SIZE(parsers)] = {0};
>   	for (unsigned int i = 0; i < ARRAY_SIZE(parsers); i++) {
>   		current = parsers[i];
>
> -		ret = current(sw, descfile);
> +		ret = current(sw, descfile, &errors[i]);
>
>   		if (ret == 0)
>   			break;
>   	}
>
>   	if (ret != 0) {
> +		for (unsigned int i = 0; i < ARRAY_SIZE(parsers); i++) {
> +			if (errors[i] != NULL) {
> +				ERROR("%s", errors[i]);
> +				free(errors[i]);
> +			}
> +		}
>   		ERROR("no parser available to parse " SW_DESCRIPTION_FILENAME "!");
>   		return ret;
>   	}
>
> +	for (unsigned int i = 0; i < ARRAY_SIZE(parsers); i++)
> +		if (errors[i] != NULL)
> +			free(errors[i]);
> +
>   	ret = check_handler_list(&sw->scripts, SCRIPT_HANDLER, IS_SCRIPT, "scripts");
>   	ret |= check_handler_list(&sw->images, IMAGE_HANDLER | FILE_HANDLER, IS_IMAGE_FILE,
>   					"images / files");
> diff --git a/include/parsers.h b/include/parsers.h
> index 0e94c2b6..7d8abc1f 100644
> --- a/include/parsers.h
> +++ b/include/parsers.h
> @@ -15,9 +15,9 @@
>   #define SW_DESCRIPTION_FILENAME	CONFIG_SWDESCRIPTION
>   #endif
>
> -typedef int (*parser_fn)(struct swupdate_cfg *swcfg, const char *filename);
> +typedef int (*parser_fn)(struct swupdate_cfg *swcfg, const char *filename, char **error);
>
>   int parse(struct swupdate_cfg *swcfg, const char *filename);
> -int parse_cfg (struct swupdate_cfg *swcfg, const char *filename);
> -int parse_json(struct swupdate_cfg *swcfg, const char *filename);
> -int parse_external(struct swupdate_cfg *swcfg, const char *filename);
> +int parse_cfg(struct swupdate_cfg *swcfg, const char *filename, char **error);
> +int parse_json(struct swupdate_cfg *swcfg, const char *filename, char **error);
> +int parse_external(struct swupdate_cfg *swcfg, const char *filename, char **error);
> diff --git a/parser/parse_external.c b/parser/parse_external.c
> index bacf4b40..eef2b09a 100644
> --- a/parser/parse_external.c
> +++ b/parser/parse_external.c
> @@ -104,7 +104,8 @@ static void sw_append_stream(struct img_type *img, const char *key,
>   		img->id.install_if_higher = 1;
>   }
>
> -int parse_external(struct swupdate_cfg *software, const char *filename)
> +int parse_external(struct swupdate_cfg *software, const char *filename,
> +		   char __attribute__((__unused__)) **error)
>   {
>   	int ret;
>   	unsigned int nstreams;
> @@ -200,8 +201,9 @@ int parse_external(struct swupdate_cfg *software, const char *filename)
>   }
>   #else
>
> -int parse_external(struct swupdate_cfg __attribute__ ((__unused__)) *software,
> -			const char __attribute__ ((__unused__)) *filename)
> +int parse_external(struct swupdate_cfg __attribute__((__unused__)) *software,
> +		   const char __attribute__((__unused__)) *filename,
> +		   char __attribute__((__unused__)) **error)
>   {
>   	return -1;
>   }
> diff --git a/parser/parser.c b/parser/parser.c
> index c7616cb8..f988dc5a 100644
> --- a/parser/parser.c
> +++ b/parser/parser.c
> @@ -1056,7 +1056,7 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
>   }
>
>   #ifdef CONFIG_LIBCONFIG
> -int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
> +int parse_cfg(struct swupdate_cfg *swcfg, const char *filename, char **error)
>   {
>   	config_t cfg;
>   	parsertype p = LIBCFG_PARSER;
> @@ -1068,8 +1068,11 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
>   	/* Read the file. If there is an error, report it and exit. */
>   	DEBUG("Parsing config file %s", filename);
>   	if(config_read_file(&cfg, filename) != CONFIG_TRUE) {
> -		ERROR("%s:%d - %s\n", config_error_file(&cfg),
> -			config_error_line(&cfg), config_error_text(&cfg));
> +		if (asprintf(error, "%s:%d - %s\n", config_error_file(&cfg),
> +			     config_error_line(&cfg), config_error_text(&cfg)) == ENOMEM_ASPRINTF) {
> +			ERROR("OOM when caching error");
> +			return -ENOMEM;
> +		}
>   		config_destroy(&cfg);
>   		return -1;
>   	}
> @@ -1084,8 +1087,9 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
>   	return ret;
>   }
>   #else
> -int parse_cfg (struct swupdate_cfg __attribute__ ((__unused__)) *swcfg,
> -		const char __attribute__ ((__unused__)) *filename)
> +int parse_cfg(struct swupdate_cfg __attribute__((__unused__)) *swcfg,
> +	      const char __attribute__((__unused__)) *filename,
> +	      char __attribute__((__unused__)) **error)
>   {
>   	return -1;
>   }
> @@ -1093,7 +1097,7 @@ int parse_cfg (struct swupdate_cfg __attribute__ ((__unused__)) *swcfg,
>
>   #define JSON_OBJECT_FREED 1
>
> -int parse_json(struct swupdate_cfg *swcfg, const char *filename)
> +int parse_json(struct swupdate_cfg *swcfg, const char *filename, char **error)
>   {
>   	int fd, ret;
>   	struct stat stbuf;
> @@ -1134,7 +1138,10 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename)
>
>   	cfg = json_tokener_parse(string);
>   	if (!cfg) {
> -		ERROR("JSON File corrupted");
> +		if (asprintf(error, "JSON File corrupted") == ENOMEM_ASPRINTF) {
> +			ERROR("OOM when caching error");
> +			return -ENOMEM;
> +		}
>   		free(string);
>   		return -1;
>   	}


Applied to -master, thanks !

Best regards,
Stefano Babic
diff mbox series

Patch

diff --git a/core/parser.c b/core/parser.c
index 02fb38a7..5cd611fc 100644
--- a/core/parser.c
+++ b/core/parser.c
@@ -151,20 +151,31 @@  int parse(struct swupdate_cfg *sw, const char *descfile)
 		return ret;
 
 #endif
+	char *errors[ARRAY_SIZE(parsers)] = {0};
 	for (unsigned int i = 0; i < ARRAY_SIZE(parsers); i++) {
 		current = parsers[i];
 
-		ret = current(sw, descfile);
+		ret = current(sw, descfile, &errors[i]);
 
 		if (ret == 0)
 			break;
 	}
 
 	if (ret != 0) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(parsers); i++) {
+			if (errors[i] != NULL) {
+				ERROR("%s", errors[i]);
+				free(errors[i]);
+			}
+		}
 		ERROR("no parser available to parse " SW_DESCRIPTION_FILENAME "!");
 		return ret;
 	}
 
+	for (unsigned int i = 0; i < ARRAY_SIZE(parsers); i++)
+		if (errors[i] != NULL)
+			free(errors[i]);
+
 	ret = check_handler_list(&sw->scripts, SCRIPT_HANDLER, IS_SCRIPT, "scripts");
 	ret |= check_handler_list(&sw->images, IMAGE_HANDLER | FILE_HANDLER, IS_IMAGE_FILE,
 					"images / files");
diff --git a/include/parsers.h b/include/parsers.h
index 0e94c2b6..7d8abc1f 100644
--- a/include/parsers.h
+++ b/include/parsers.h
@@ -15,9 +15,9 @@ 
 #define SW_DESCRIPTION_FILENAME	CONFIG_SWDESCRIPTION
 #endif
 
-typedef int (*parser_fn)(struct swupdate_cfg *swcfg, const char *filename);
+typedef int (*parser_fn)(struct swupdate_cfg *swcfg, const char *filename, char **error);
 
 int parse(struct swupdate_cfg *swcfg, const char *filename);
-int parse_cfg (struct swupdate_cfg *swcfg, const char *filename);
-int parse_json(struct swupdate_cfg *swcfg, const char *filename);
-int parse_external(struct swupdate_cfg *swcfg, const char *filename);
+int parse_cfg(struct swupdate_cfg *swcfg, const char *filename, char **error);
+int parse_json(struct swupdate_cfg *swcfg, const char *filename, char **error);
+int parse_external(struct swupdate_cfg *swcfg, const char *filename, char **error);
diff --git a/parser/parse_external.c b/parser/parse_external.c
index bacf4b40..eef2b09a 100644
--- a/parser/parse_external.c
+++ b/parser/parse_external.c
@@ -104,7 +104,8 @@  static void sw_append_stream(struct img_type *img, const char *key,
 		img->id.install_if_higher = 1;
 }
 
-int parse_external(struct swupdate_cfg *software, const char *filename)
+int parse_external(struct swupdate_cfg *software, const char *filename,
+		   char __attribute__((__unused__)) **error)
 {
 	int ret;
 	unsigned int nstreams;
@@ -200,8 +201,9 @@  int parse_external(struct swupdate_cfg *software, const char *filename)
 }
 #else
 
-int parse_external(struct swupdate_cfg __attribute__ ((__unused__)) *software,
-			const char __attribute__ ((__unused__)) *filename)
+int parse_external(struct swupdate_cfg __attribute__((__unused__)) *software,
+		   const char __attribute__((__unused__)) *filename,
+		   char __attribute__((__unused__)) **error)
 {
 	return -1;
 }
diff --git a/parser/parser.c b/parser/parser.c
index c7616cb8..f988dc5a 100644
--- a/parser/parser.c
+++ b/parser/parser.c
@@ -1056,7 +1056,7 @@  static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
 }
 
 #ifdef CONFIG_LIBCONFIG
-int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
+int parse_cfg(struct swupdate_cfg *swcfg, const char *filename, char **error)
 {
 	config_t cfg;
 	parsertype p = LIBCFG_PARSER;
@@ -1068,8 +1068,11 @@  int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
 	/* Read the file. If there is an error, report it and exit. */
 	DEBUG("Parsing config file %s", filename);
 	if(config_read_file(&cfg, filename) != CONFIG_TRUE) {
-		ERROR("%s:%d - %s\n", config_error_file(&cfg),
-			config_error_line(&cfg), config_error_text(&cfg));
+		if (asprintf(error, "%s:%d - %s\n", config_error_file(&cfg),
+			     config_error_line(&cfg), config_error_text(&cfg)) == ENOMEM_ASPRINTF) {
+			ERROR("OOM when caching error");
+			return -ENOMEM;
+		}
 		config_destroy(&cfg);
 		return -1;
 	}
@@ -1084,8 +1087,9 @@  int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
 	return ret;
 }
 #else
-int parse_cfg (struct swupdate_cfg __attribute__ ((__unused__)) *swcfg,
-		const char __attribute__ ((__unused__)) *filename)
+int parse_cfg(struct swupdate_cfg __attribute__((__unused__)) *swcfg,
+	      const char __attribute__((__unused__)) *filename,
+	      char __attribute__((__unused__)) **error)
 {
 	return -1;
 }
@@ -1093,7 +1097,7 @@  int parse_cfg (struct swupdate_cfg __attribute__ ((__unused__)) *swcfg,
 
 #define JSON_OBJECT_FREED 1
 
-int parse_json(struct swupdate_cfg *swcfg, const char *filename)
+int parse_json(struct swupdate_cfg *swcfg, const char *filename, char **error)
 {
 	int fd, ret;
 	struct stat stbuf;
@@ -1134,7 +1138,10 @@  int parse_json(struct swupdate_cfg *swcfg, const char *filename)
 
 	cfg = json_tokener_parse(string);
 	if (!cfg) {
-		ERROR("JSON File corrupted");
+		if (asprintf(error, "JSON File corrupted") == ENOMEM_ASPRINTF) {
+			ERROR("OOM when caching error");
+			return -ENOMEM;
+		}
 		free(string);
 		return -1;
 	}