setup: return failed desc _path_ from setup_interactively()

Returning the failed desc was just a lazy half-assed thing that
was sort of the best option in the simpler, pre-paths world.

But now that everything has paths thanks to recursive settings,
let's just return the path to the failed setting's desc.

This conveniently gets rid of a UAF bug when setup returned the
setting->desc as the failed desc, and main would print the desc
*after* freeing all the settings in its final moments.

But the best part is now more of the errors parsing settings
should be accompanied by an illuminatingly relevant setting path.

Previously you'd at best get a bare key from the desc, but often
no failed desc was returned at all and you saw no guidance at
all.  But with the recent improvements to the setup error
handling I think those cases should be few if not entirely
eliminated.

3 files changed, 41 insertions, 24 deletions

diff --git a/src/main.c b/src/main.c
index 119931b..44bb735 100644
--- a/src/main.c
+++ b/src/main.c
@@ -199,7 +199,7 @@ static int parse_seed(const char *in, unsigned *res_seed)
 /* turn args into settings, automatically applying defaults if appropriate, or interactively if appropriate. */
 /* returns negative value on error, 0 when settings unchanged from args, 1 when changed */
 /* on error, *res_failed_desc _may_ be assigned with something useful. */
-static int setup_from_args(til_args_t *args, setup_t *res_setup, const til_setting_desc_t **res_failed_desc)
+static int setup_from_args(til_args_t *args, setup_t *res_setup, const char **res_failed_desc_path)
 {
 	int	r = -ENOMEM, changes = 0;
 	setup_t	setup = { .seed = time(NULL) + getpid() };
@@ -228,13 +228,13 @@ static int setup_from_args(til_args_t *args, setup_t *res_setup, const til_setti
 	if (!setup.video_settings)
 		goto _err;
 
-	r = setup_interactively(setup.module_settings, til_module_setup, args->use_defaults, &setup.module_setup, res_failed_desc);
+	r = setup_interactively(setup.module_settings, til_module_setup, args->use_defaults, &setup.module_setup, res_failed_desc_path);
 	if (r < 0)
 		goto _err;
 	if (r)
 		changes = 1;
 
-	r = setup_interactively(setup.video_settings, setup_video, args->use_defaults, &setup.video_setup, res_failed_desc);
+	r = setup_interactively(setup.video_settings, setup_video, args->use_defaults, &setup.video_setup, res_failed_desc_path);
 	if (r < 0)
 		goto _err;
 	if (r)
@@ -361,9 +361,9 @@ static void * rototiller_thread(void *_rt)
  */
 int main(int argc, const char *argv[])
 {
-	const til_setting_desc_t	*failed_desc = NULL;
-	setup_t				setup = {};
-	int				r;
+	const char	*failed_desc_path = NULL;
+	setup_t		setup = {};
+	int		r;
 
 	exit_if((r = til_init()) < 0,
 		"unable to initialize libtil: %s", strerror(-r));
@@ -374,11 +374,11 @@ int main(int argc, const char *argv[])
 	if (rototiller.args.help)
 		return print_help() < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
 
-	exit_if((r = setup_from_args(&rototiller.args, &setup, &failed_desc)) < 0,
+	exit_if((r = setup_from_args(&rototiller.args, &setup, &failed_desc_path)) < 0,
 		"unable to use args%s%s%s: %s",
-		failed_desc ? " for setting \"" : "",
-		failed_desc ? failed_desc->spec.key : "",
-		failed_desc ? "\"" : "",
+		failed_desc_path ? " for setting \"" : "",
+		failed_desc_path ? : "",
+		failed_desc_path ? "\"" : "", /* XXX: technically leaking the path when set, oh well */
 		strerror(-r));
 
 	exit_if(r && print_setup_as_args(&setup, !rototiller.args.gogogo) < 0,
diff --git a/src/setup.c b/src/setup.c
index 131c856..c607f13 100644
--- a/src/setup.c
+++ b/src/setup.c
@@ -4,11 +4,34 @@
 #include "setup.h"
 #include "til_settings.h"
 #include "til_setup.h"
+#include "til_str.h"
 #include "til_util.h"
 
 
+/* Helper for turning the failed desc into a char * and storing it in the provided place before propagating
+ * the error return value
+ */
+static int setup_ret_failed_desc_path(const til_setting_desc_t *failed_desc, int r, const char **res_failed_desc_path)
+{
+	til_str_t	*str;
+
+	assert(res_failed_desc_path);
+
+	str = til_str_new("");
+	if (!str)
+		return r;
+
+	if (til_setting_desc_strprint_path(failed_desc, str) < 0)
+		return r;
+
+	*res_failed_desc_path = til_str_to_buf(str, NULL);
+
+	return r;
+}
+
+
 /* returns negative on error, otherwise number of additions made to settings */
-int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_settings_t *settings, til_setting_t **res_setting, const til_setting_desc_t **res_desc, til_setup_t **res_setup), int defaults, til_setup_t **res_setup, const til_setting_desc_t **res_failed_desc)
+int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_settings_t *settings, til_setting_t **res_setting, const til_setting_desc_t **res_desc, til_setup_t **res_setup), int defaults, til_setup_t **res_setup, const char **res_failed_desc_path)
 {
 	unsigned			additions = 0;
 	char				buf[256] = "\n";
@@ -52,11 +75,8 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se
 			}
 
 			r = til_setting_spec_check(&desc->spec, setting->value);
-			if (r < 0) {
-				*res_failed_desc = desc;
-
-				return r;
-			}
+			if (r < 0)
+				return setup_ret_failed_desc_path(desc, r, res_failed_desc_path);
 
 			if (desc->spec.as_nested_settings && !setting->value_as_nested_settings) {
 				char	*label = NULL;
@@ -71,12 +91,9 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se
 				setting->value_as_nested_settings = til_settings_new(NULL, desc->container, desc->spec.key ? : label, setting->value);
 				free(label);
 
-				if (!setting->value_as_nested_settings) {
-					*res_failed_desc = desc;
-
-					/* FIXME: til_settings_new() seems like it should return an errno, since it can encounter parse errors too? */
-					return -ENOMEM;
-				};
+				/* FIXME: til_settings_new() seems like it should return an errno, since it can encounter parse errors too? */
+				if (!setting->value_as_nested_settings)
+					return setup_ret_failed_desc_path(desc, -ENOMEM, res_failed_desc_path);
 			}
 
 			setting->desc = desc;
@@ -177,7 +194,7 @@ _next:
 
 	if (r < 0) {
 		if (r == -EINVAL)
-			*res_failed_desc = setting->desc;
+			return setup_ret_failed_desc_path(setting->desc, r, res_failed_desc_path);
 
 		return r;
 	}
diff --git a/src/setup.h b/src/setup.h
index 0862e45..484cc3e 100644
--- a/src/setup.h
+++ b/src/setup.h
@@ -4,6 +4,6 @@
 #include "til_settings.h"
 #include "til_setup.h"
 
-int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_settings_t *settings, til_setting_t **res_setting, const til_setting_desc_t **res_desc, til_setup_t **res_setup), int defaults, til_setup_t **res_setup, const til_setting_desc_t **res_failed_desc);
+int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_settings_t *settings, til_setting_t **res_setting, const til_setting_desc_t **res_desc, til_setup_t **res_setup), int defaults, til_setup_t **res_setup, const char **res_failed_desc_path);
 
 #endif