Igt-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Lucas De Marchi <lucas.demarchi@intel.com>
To: igt-dev@lists.freedesktop.org
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Subject: [PATCH i-g-t v3 07/10] runner: Fix use of newline on arguments
Date: Mon,  3 Feb 2025 10:09:04 -0800	[thread overview]
Message-ID: <20250203182141.617229-1-lucas.demarchi@intel.com> (raw)
In-Reply-To: <yeb7toxw7aqtrwlncbuhsuhdlpmckwve2iwhz2crep62fqdifn@by2yiutcan4f>

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


  reply	other threads:[~2025-02-03 18:22 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-30 17:21 [PATCH i-g-t v3 00/10] Add igt_runner's cmdline to results Lucas De Marchi
2025-01-30 17:21 ` [PATCH i-g-t v3 01/10] runner/settings: Fix code_coverage_script leak Lucas De Marchi
2025-01-30 17:21 ` [PATCH i-g-t v3 02/10] runner: Free settings at the end Lucas De Marchi
2025-01-30 17:21 ` [PATCH i-g-t v3 03/10] runner/settings: Deduplicate cleanup Lucas De Marchi
2025-01-30 17:21 ` [PATCH i-g-t v3 04/10] runner/settings: Use wrapper macros for each type Lucas De Marchi
2025-02-07 18:34   ` Gustavo Sousa
2025-01-30 17:21 ` [PATCH i-g-t v3 05/10] runner/settings: Match serialization to parse Lucas De Marchi
2025-02-07 19:13   ` Gustavo Sousa
2025-01-30 17:21 ` [PATCH i-g-t v3 06/10] runner/settings: Drop extra strdup Lucas De Marchi
2025-01-30 17:21 ` [PATCH i-g-t v3 07/10] runner: Fix use of newline on arguments Lucas De Marchi
2025-01-31  9:21   ` Peter Senna Tschudin
2025-01-31 15:58     ` Lucas De Marchi
2025-02-03 18:09       ` Lucas De Marchi [this message]
2025-02-07 20:15         ` Gustavo Sousa
2025-01-30 17:21 ` [PATCH i-g-t v3 08/10] runner/settings: Add helpers to serialize/parse array Lucas De Marchi
2025-02-07 20:26   ` Gustavo Sousa
2025-01-30 17:21 ` [PATCH i-g-t v3 09/10] runner/settings: Serialize command line Lucas De Marchi
2025-02-07 20:36   ` Gustavo Sousa
2025-02-07 20:41     ` Gustavo Sousa
2025-01-30 17:21 ` [PATCH i-g-t v3 10/10] runner/resultgen: Add cmdline to results.json Lucas De Marchi
2025-01-30 19:00   ` Kamil Konieczny
2025-01-30 19:17     ` Lucas De Marchi
2025-01-31  9:31       ` Peter Senna Tschudin
2025-01-31 12:16       ` Kamil Konieczny
2025-01-31 15:39         ` Lucas De Marchi
2025-01-30 19:12 ` ✓ Xe.CI.BAT: success for Add igt_runner's cmdline to results (rev3) Patchwork
2025-01-30 19:12 ` ✓ i915.CI.BAT: " Patchwork
2025-01-30 21:21 ` ✗ i915.CI.Full: failure " Patchwork
2025-01-30 22:06 ` ✓ Xe.CI.Full: success " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250203182141.617229-1-lucas.demarchi@intel.com \
    --to=lucas.demarchi@intel.com \
    --cc=igt-dev@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox