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

[Lucas De Marchi]

On Fri, Jan 31, 2025 at 10:21:33AM +0100, Peter Senna Tschudin wrote:
>
>
>On 30.01.2025 18:21, Lucas De Marchi wrote:
>> 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.
>>
>> For compatibility with previous parsing, it still prints "(null)" for
>> NULL pointers: ideally there would be nothing printed to avoid the
>> special case for this string.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  runner/settings.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/runner/settings.c b/runner/settings.c
>> index 693c5484e..b527c01d9 100644
>> --- a/runner/settings.c
>> +++ b/runner/settings.c
>> @@ -1052,10 +1052,79 @@ 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)
>> +{
>> +	if (!s) {
>> +		fputs("(null)", f);
>> +		return;
>> +	}
>> +
>> +	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
>
>Nit: there is a specific format for documenting new functions. I have the impression
>that the arguments are not properly documented.

For external functions for other parts of igt to use, the format is
kernel-doc. That is not the case for static functions.

>
>> + *
>> + * 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 size_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 (*s == '\\' && next == 'x') {
>> +			s += 2;
>> +			buf[dst_len++] = strtoul(s, &s, 16);
>> +			if (*s != 'h') {
>> +				/* this shouldn't happen */
>> +				return 0;
>> +			}
>> +			s++;
>> +		} else {
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	buf[dst_len] = '\0';
>> +
>> +	if (n_src)
>> +		*n_src = s - buf;
>> +
>> +	return dst_len;
>> +}
>
>Have you considered  json_tokener_parse() / json_object_get_string() ?
>Glib' g_strescape () / g_strcompress() ? Not a fan of Glib,
>but it is currently used anyway. Or even libcurl' curl_easy_escape() /
>curl_easy_unescape()? Meson probes for the lib, but I did not see it in use.

depending on other things I'm working on, I will evaluate moving this file
to toml so I don't like adding another dep.

>
>My point being that it would be better to reuse instead of reinventing this
>wheel. If you prefer to use your own, please add unit testing, I found at least
>one issue:
>
>Lorem ipsum dolor sit amet
>Lorem ipsum dolor sit amet (26)
>
>Lorem \xZZh ipsum dolor sit amet
>Lorem  (0)
>
>Lorem \x41h ipsum dolor sit amet
>Lorem A ipsum dolor sit amet (32)

sorry, but I'm not following the example. What string was
encoded by escape_str() and decoded wrongly by unescape_str()? Not sure
there's much value making it very fool proof.

Additional tests would check that "given a random string s, does
escape_str(s) -> unescape_str(s) produce back the same string?", but I'm
just relying on the ones already there for hooks.

unescape_str() doesn't accept random strings, only the ones escaped by
its counterpart function. I deliberately didn't bother making it super
complex, see e.g. `/* this shouldn't happen */` above.  But I will see
what additional checks I can add.


>
>I guess you want to check strtoul() output for errors(can it cause an infinite
>loop on error?), check buffer limits to prevent writing past allocated size,

output is never bigger than input when unescaping a valid string.

>and using error code such as -1 instead of failing with 0.

yep

>
>> +
>>  #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 {	\
>> +		fputs(#name " : ", f);	\
>> +		escape_str(s->name, f);	\
>> +		fputc('\n', f);		\
>> +	} while (0)
>>  bool serialize_settings(struct settings *settings)
>>  {
>>  	FILE *f;
>> @@ -1171,6 +1240,12 @@ static char *parse_str(char **val)
>>  {
>>  	char *ret = *val;
>>
>> +	/*
>> +	 * 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
>> +	 */
>> +	unescape_str(ret, NULL);
>>  	*val = NULL;
>>
>>  	return ret;
>