Re: [PATCH i-g-t v3 07/10] runner: Fix use of newline on arguments

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Lucas De Marchi (2025-02-03 15:09:04-03:00)
>Currently there's no support for newlines on arguments passed to runner.
>However it's also a silent failure:
>
>        # igt_runner --test-list '/tmp/test
>        list2.txt' build/tests/ /tmp/results
>
>        # head /tmp/results/metadata.txt
>        disk_usage_limit : 0
>        test_list : /tmp/test
>        list2.txt
>        name : results
>        ...
>
>        # ./build/runner/igt_resume /tmp/results
>        [9840425.334900] All tests already executed.
>        resume failed at generating results
>        Done.
>
>Embedding a newline like this is very dubious for test-list, but it's
>used for e.g. hooks. In future we will add the command line to the
>metadata and possibly migrate the hooks, so add support for
>escaping/unescaping the string on save/restore.
>
>The method chosen is slightly different than the one used for hooks:
>instead of adding a escape char and keeping the char escaped, this just
>prefers using an hex representation of the char with a \x<HEX>h
>sequence. This makes it easier when unescaping since the reader can
>continue reading one line per iteration. In future this can also be
>adopted by the hooks or even migrating the hooks to use metadata.txt.
>
>Another fix is that now we just skip null values on the serialization
>side.  Previously it would serialize "(null)" and then load that string
>instead of NULL. Add code_coverage_script to the runner_test to cover
>that, which would previously fail.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
>
>- Fix NULL string (bug already present)
>- Add sanity check on the unescape_str() side to differentiate invalid
>  from empty strings. Now unescape_str() returns negative in cases of
>  error and there's better validation for the escaped char
>
> runner/runner_tests.c |  1 +
> runner/settings.c     | 85 +++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 83 insertions(+), 3 deletions(-)
>
>diff --git a/runner/runner_tests.c b/runner/runner_tests.c
>index 8441763f2..93b3ebc9f 100644
>--- a/runner/runner_tests.c
>+++ b/runner/runner_tests.c
>@@ -200,6 +200,7 @@ static void assert_settings_equal(struct settings *one, struct settings *two)
>         igt_assert_eq(one->use_watchdog, two->use_watchdog);
>         igt_assert_eqstr(one->test_root, two->test_root);
>         igt_assert_eqstr(one->results_path, two->results_path);
>+        igt_assert_eqstr(one->code_coverage_script, two->code_coverage_script);
>         igt_assert_eq(one->piglit_style_dmesg, two->piglit_style_dmesg);
>         igt_assert_eq(one->dmesg_warn_level, two->dmesg_warn_level);
>         igt_assert_eq(one->prune_mode, two->prune_mode);
>diff --git a/runner/settings.c b/runner/settings.c
>index 693c5484e..03402a385 100644
>--- a/runner/settings.c
>+++ b/runner/settings.c
>@@ -7,6 +7,7 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <getopt.h>
>+#include <inttypes.h>
> #include <libgen.h>
> #include <limits.h>
> #include <stdio.h>
>@@ -1052,10 +1053,80 @@ static bool serialize_hook_strs(struct settings *settings, int dirfd)
>         return true;
> }
> 
>+/*
>+ * Serialize @s to @f, escaping '\' and '\n'. See unescape_str()
>+ */
>+static void escape_str(const char *s, FILE *f)
>+{
>+        while (*s) {
>+                size_t len = strcspn(s, "\\\n");
>+
>+                if (len > 0) {
>+                        fwrite(s, len, 1, f);
>+                        s += len;
>+                }
>+
>+                if (*s) {
>+                        fprintf(f, "\\x%xh", *s);

I was thinking we could have used %02x and avoided the "h" suffix... But
I guess the "h" suffix makes the unescape logic simpler.

>+                        s++;
>+                }
>+        }
>+}
>+
>+/*
>+ * Unescape a '\' and '\n': undo escape_str
>+ *
>+ * Exacpe chars using the form '\x<hex>h' so they don't interfere with the line

Exacpe?

>+ * parser.
>+ *
>+ * Return the number of chars saved in buf and optionally
>+ * the number of chars scanned in @n_src if that is non-nul.
>+ */
>+static ssize_t unescape_str(char *buf, size_t *n_src)
>+{
>+        size_t dst_len = 0;
>+        char *s = buf;
>+
>+        while (*s) {
>+                char next = *(s + 1);
>+
>+                if (*s != '\\') {
>+                        buf[dst_len++] = *s++;
>+                } else if (next == 'x') {
>+                        unsigned long num;
>+
>+                        s += 2;
>+
>+                        num = strtoul(s, &s, 16);

Well, I think strtoul() is too permissive here, it allows the str to
start with whitespaces and some prefixes that are not generated by
escape_str().

But I guess that's acceptable if we only care about unescape_str() being
able to process what was generated by escape_str(), as you commented
earlier.

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>+                        /* cover both error due to overflow or invalid char */
>+                        if (num > UINT8_MAX || *s != 'h')
>+                                return -EINVAL;
>+
>+                        buf[dst_len++] = num;
>+                        s++;
>+                } else {
>+                        return -EINVAL;
>+                }
>+        }
>+
>+        buf[dst_len] = '\0';
>+
>+        if (n_src)
>+                *n_src = s - buf;
>+
>+        return dst_len;
>+}
>+
> #define SERIALIZE_LINE(f, s, name, fmt) fprintf(f, "%s : " fmt "\n", #name, s->name)
> #define SERIALIZE_INT(f, s, name) SERIALIZE_LINE(f, s, name, "%d")
> #define SERIALIZE_UL(f, s, name) SERIALIZE_LINE(f, s, name, "%lu")
>-#define SERIALIZE_STR(f, s, name) SERIALIZE_LINE(f, s, name, "%s")
>+#define SERIALIZE_STR(f, s, name) do {                \
>+                if (s->name) {                        \
>+                        fputs(#name " : ", f);        \
>+                        escape_str(s->name, f);        \
>+                        fputc('\n', f);                \
>+                }                                \
>+        } while (0)
> bool serialize_settings(struct settings *settings)
> {
>         FILE *f;
>@@ -1171,9 +1242,17 @@ static char *parse_str(char **val)
> {
>         char *ret = *val;
> 
>-        *val = NULL;
>+        /*
>+         * Unescaping a string is guaranteed to produce a string that is
>+         * smaller or of the same size. Just modify it in place and leak the
>+         * buffer
>+         */
>+        if (unescape_str(ret, NULL) >= 0) {
>+                *val = NULL;
>+                return ret;
>+        }
> 
>-        return ret;
>+        return NULL;
> }
> 
> #define PARSE_LINE(s, name, val, field, _f)        \
>-- 
>2.48.1
>