From ba8800df2c9deea446cb4fde7a3f0634b1f0084c Mon Sep 17 00:00:00 2001
From: Jonas 'Sortie' Termansen <sortie@maxsi.org>
Date: Sun, 17 Jan 2021 22:54:35 +0100
Subject: [PATCH] Improve error handling in upgrade.conf(5) parsing.

---
 sysinstall/conf.c       | 76 +++++++++++++++++++++++++++--------------
 sysinstall/conf.h       |  6 ++--
 sysinstall/sysinstall.c | 36 ++++++++-----------
 sysinstall/sysmerge.c   |  4 ++-
 sysinstall/sysupgrade.c |  5 ++-
 5 files changed, 77 insertions(+), 50 deletions(-)

diff --git a/sysinstall/conf.c b/sysinstall/conf.c
index bc8d5483..1876398c 100644
--- a/sysinstall/conf.c
+++ b/sysinstall/conf.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2016 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2015, 2016, 2017, 2021 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -28,54 +28,67 @@
 
 #include "conf.h"
 
-static bool conf_boolean(const char* name, const char* value, const char* path)
+void conf_init(struct conf* conf)
+{
+	memset(conf, 0, sizeof(*conf));
+	conf->ports = true;
+	conf->system = true;
+}
+
+void conf_free(struct conf* conf)
+{
+	conf_init(conf);
+}
+
+static bool conf_boolean(const char* name,
+                         const char* value,
+                         const char* path,
+                         off_t line_number)
 {
 	if ( !strcmp(value, "yes") )
 		return true;
 	if ( !strcmp(value, "no") )
 		return false;
-	printf("%s: %s: Expected yes or no instead of unsupported value\n",
-	       path, name);
+	printf("%s:%ji: %s: Expected yes or no instead of unsupported value\n",
+	       path, (intmax_t) line_number, name);
 	return false;
 }
 
-static void conf_assign(struct conf* conf,
+static bool conf_assign(struct conf* conf,
                         const char* name,
                         const char* value,
-                        const char* path)
+                        const char* path,
+                        off_t line_number)
 {
 	if ( !strcmp(name, "grub") )
-		conf->grub = conf_boolean(name, value, path);
+		conf->grub = conf_boolean(name, value, path, line_number);
 	else if ( !strcmp(name, "newsrc") )
-		conf->newsrc = conf_boolean(name, value, path);
+		conf->newsrc = conf_boolean(name, value, path, line_number);
 	else if ( !strcmp(name, "ports") )
-		conf->ports = conf_boolean(name, value, path);
+		conf->ports = conf_boolean(name, value, path, line_number);
 	else if ( !strcmp(name, "src") )
-		conf->src = conf_boolean(name, value, path);
+		conf->src = conf_boolean(name, value, path, line_number);
 	else if ( !strcmp(name, "system") )
-		conf->system = conf_boolean(name, value, path);
+		conf->system = conf_boolean(name, value, path, line_number);
 	else
-		printf("%s: %s: Unsupported variable\n", path, name);
+		printf("%s:%ji: Unsupported variable: %s\n", path,
+		       (intmax_t) line_number, name);
+	return true;
 }
 
-void load_upgrade_conf(struct conf* conf, const char* path)
+bool conf_load(struct conf* conf, const char* path)
 {
-	memset(conf, 0, sizeof(*conf));
-	conf->ports = true;
-	conf->system = true;
-
 	FILE* fp = fopen(path, "r");
 	if ( !fp )
-	{
-		if ( errno == ENOENT )
-			return;
-		err(2, "%s", path);
-	}
+		return false;
 	char* line = NULL;
 	size_t line_size = 0;
 	ssize_t line_length;
+	intmax_t line_number = 0;
+	bool success = true;
 	while ( 0 < (line_length = getline(&line, &line_size, fp)) )
 	{
+		line_number++;
 		if ( line[line_length - 1] == '\n' )
 			line[--line_length] = '\0';
 		line_length = 0;
@@ -87,8 +100,13 @@ void load_upgrade_conf(struct conf* conf, const char* path)
 		char* name = line;
 		while ( *name && isblank((unsigned char) *name) )
 			name++;
-		if ( !*name || *name == '=' )
+		if ( !*name )
 			continue;
+		if ( *name == '=' )
+		{
+			printf("%s:%ji: Ignoring malformed line\n", path, line_number);
+			continue;
+		}
 		size_t name_length = 1;
 		while ( name[name_length] &&
 		        !isblank((unsigned char) name[name_length]) &&
@@ -98,15 +116,23 @@ void load_upgrade_conf(struct conf* conf, const char* path)
 		while ( *value && isblank((unsigned char) *value) )
 			value++;
 		if ( *value != '=' )
+		{
+			printf("%s:%ji: Ignoring malformed line\n", path, line_number);
 			continue;
+		}
 		value++;
 		while ( *value && isblank((unsigned char) *value) )
 			value++;
 		name[name_length] = '\0';
-		conf_assign(conf, name, value, path);
+		if ( !conf_assign(conf, name, value, path, line_number) )
+		{
+			success = false;
+			break;
+		}
 	}
 	if ( ferror(fp) )
-		err(2, "%s", path);
+		success = false;
 	free(line);
 	fclose(fp);
+	return success;
 }
diff --git a/sysinstall/conf.h b/sysinstall/conf.h
index 43908ae5..761df306 100644
--- a/sysinstall/conf.h
+++ b/sysinstall/conf.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2016 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2015, 2016, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -29,6 +29,8 @@ struct conf
 	bool system;
 };
 
-void load_upgrade_conf(struct conf* conf, const char* path);
+void conf_init(struct conf* conf);
+void conf_free(struct conf* conf);
+bool conf_load(struct conf* conf, const char* path);
 
 #endif
diff --git a/sysinstall/sysinstall.c b/sysinstall/sysinstall.c
index b2c34179..08833770 100644
--- a/sysinstall/sysinstall.c
+++ b/sysinstall/sysinstall.c
@@ -142,27 +142,16 @@ static bool should_install_bootloader_path(const char* mnt,
 		warn("malloc");
 		return false;
 	}
-	// TODO: The load_upgrade_conf function might exit the process on failure,
-	//       but we don't want that. Redesign the mountpoint code so the caller
-	//       controls this.
-	pid_t pid = fork();
-	if ( pid < 0 )
-	{
-		warn("fork");
-		free(conf_path);
-		return false;
-	}
-	if ( !pid )
-	{
-		struct conf conf;
-		load_upgrade_conf(&conf, conf_path);
-		bool should = conf.grub;
-		_exit(should ? 0 : 1);
-	}
-	int status;
-	if ( waitpid(pid, &status, 0) < 0 )
-		return false;
-	return WIFEXITED(status) && WEXITSTATUS(status) == 0;
+	struct conf conf;
+	conf_init(&conf);
+	bool result = false;
+	if ( conf_load(&conf, conf_path) )
+		result = conf.grub;
+	else if ( errno != ENOENT )
+		warn("%s: /etc/upgrade.conf", path_of_blockdevice(bdev));
+	conf_free(&conf);
+	free(conf_path);
+	return result;
 }
 
 static bool should_install_bootloader_bdev(struct blockdevice* bdev)
@@ -412,6 +401,11 @@ int main(void)
 	struct utsname uts;
 	uname(&uts);
 
+	struct conf conf;
+	conf_init(&conf);
+	if ( !conf_load(&conf, "/etc/upgrade.conf") && errno != ENOENT )
+		warn("/etc/upgrade.conf");
+
 	static char input[256];
 
 	textf("Hello and welcome to the " BRAND_DISTRIBUTION_NAME " " VERSIONSTR ""
diff --git a/sysinstall/sysmerge.c b/sysinstall/sysmerge.c
index 3b60d3f4..7a6acc18 100644
--- a/sysinstall/sysmerge.c
+++ b/sysinstall/sysmerge.c
@@ -241,7 +241,9 @@ int main(int argc, char* argv[])
 	// TODO: Check for version (skipping, downgrading).
 
 	struct conf conf;
-	load_upgrade_conf(&conf, "/etc/upgrade.conf");
+	conf_init(&conf);
+	if ( !conf_load(&conf, "/etc/upgrade.conf") && errno == ENOENT )
+		err(2, "/etc/upgrade.conf");
 
 	bool can_run_new_abi =
 		abi_compatible(new_release.abi_major, new_release.abi_minor,
diff --git a/sysinstall/sysupgrade.c b/sysinstall/sysupgrade.c
index 329a8e40..87d55595 100644
--- a/sysinstall/sysupgrade.c
+++ b/sysinstall/sysupgrade.c
@@ -721,9 +721,12 @@ int main(void)
 
 	bool do_upgrade_bootloader;
 	struct conf conf;
+	conf_init(&conf);
 	while ( true )
 	{
-		load_upgrade_conf(&conf, "etc/upgrade.conf");
+		conf_free(&conf);
+		if ( !conf_load(&conf, "etc/upgrade.conf") && errno != ENOENT )
+			err(2, "etc/upgrade.conf");
 
 		do_upgrade_bootloader =
 			conf.grub && (conf.ports || (conf.system && can_run_old_abi));