Re: [PATCH 05/12] util/error: allow non-NUL-terminated err->src
Paolo Bonzini writes:
> On 5/27/25 15:42, Markus Armbruster wrote:
>> Paolo Bonzini writes:
>>
>>> Rust makes the current file available as a statically-allocated string,
>>> but without a NUL terminator. Allow this by storing an optional maximum
>>> length in the Error.
>>>
>>> Note that for portability I am not relying on fprintf's precision
>>> specifier not accessing memory beyond what will be printed.
>> Can you elaborate on the portability problem? I figure ...
>>
>>> {
>>> if (errp == &error_abort) {
>>> +const char *src = err->src;
>>> +if (err->src_len >= 0) {
>>> +/* No need to free it, the program will abort very soon... */
>>> +src = g_strndup(err->src, err->src_len);
>>> +}
>>> fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>>> -err->func, err->src, err->line);
>>> +err->func, src, err->line);
>> ... you're avoiding the simpler
>> fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
>> err->func, err->src_len, err->src, err->line);
>> because of it.
>
> I couldn't find anything that says %s is allowed to not be NUL-terminated if
> a precision is given. That is, whether something like this:
>
>char foo[] = {'H', 'e', 'l', 'l', 'o'};
>printf("%.5s\n", foo);
>
> is guaranteed to work.
>From ISO/IEC 9899:1999 ยง7.19.6.1 "The fprintf function":
[#8] The conversion specifiers and their meanings are:
[...]
s If no l length modifier is present, the argument
shall be a pointer to the initial element of an
array of character type.237) Characters from the
array are written up to (but not including) the
terminating null character. If the precision is
specified, no more than that many bytes are written.
-->If the precision is not specified or is greater than
-->the size of the array, the array shall contain a
-->null character.
237No special provisions are made for multibyte characters.
This clearly implies that the string need not be null-terminated when a
suitable precision is specified. Which it is here.
> This is opposed to:
>
> 1) strnlen
> (https://pubs.opengroup.org/onlinepubs/9699919799/functions/strnlen.html),
> which is guaranteed to examine no more than the number of bytes given by the
> second character;
>
> 2) strndup, for which I found at least a clarification at
> https://www.austingroupbugs.net/view.php?id=1397.
>
> 3) g_strndup, which guarantees that the allocated block is of length n+1 and
> padded with NULs (though in the case above there will be just one NUL anyway)
>
> And also, for strndup/g_strndup it would be quite asinine to implement it
> using some kind of min(strlen(s), n) but for printf the complexity is greater
> so you never know. I erred on the side of caution because avoiding an
> allocation before an abort() isn't particularly interesting.
Keeping the code simple is always interesting, though :)
Re: [PATCH 05/12] util/error: allow non-NUL-terminated err->src
On 5/27/25 15:42, Markus Armbruster wrote:
Paolo Bonzini writes:
Rust makes the current file available as a statically-allocated string,
but without a NUL terminator. Allow this by storing an optional maximum
length in the Error.
Note that for portability I am not relying on fprintf's precision
specifier not accessing memory beyond what will be printed.
Can you elaborate on the portability problem? I figure ...
{
if (errp == &error_abort) {
+const char *src = err->src;
+if (err->src_len >= 0) {
+/* No need to free it, the program will abort very soon... */
+src = g_strndup(err->src, err->src_len);
+}
fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
-err->func, err->src, err->line);
+err->func, src, err->line);
... you're avoiding the simpler
fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
err->func, err->src_len, err->src, err->line);
because of it.
I couldn't find anything that says %s is allowed to not be
NUL-terminated if a precision is given. That is, whether something like
this:
char foo[] = {'H', 'e', 'l', 'l', 'o'};
printf("%.5s\n", foo);
is guaranteed to work.
This is opposed to:
1) strnlen
(https://pubs.opengroup.org/onlinepubs/9699919799/functions/strnlen.html),
which is guaranteed to examine no more than the number of bytes given by
the second character;
2) strndup, for which I found at least a clarification at
https://www.austingroupbugs.net/view.php?id=1397.
3) g_strndup, which guarantees that the allocated block is of length n+1
and padded with NULs (though in the case above there will be just one
NUL anyway)
And also, for strndup/g_strndup it would be quite asinine to implement
it using some kind of min(strlen(s), n) but for printf the complexity is
greater so you never know. I erred on the side of caution because
avoiding an allocation before an abort() isn't particularly interesting.
Paolo
Re: [PATCH 05/12] util/error: allow non-NUL-terminated err->src
Paolo Bonzini writes:
> Rust makes the current file available as a statically-allocated string,
> but without a NUL terminator. Allow this by storing an optional maximum
> length in the Error.
>
> Note that for portability I am not relying on fprintf's precision
> specifier not accessing memory beyond what will be printed.
Can you elaborate on the portability problem? I figure ...
> Signed-off-by: Paolo Bonzini
> ---
> include/qapi/error-internal.h | 1 +
> util/error.c | 8 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/error-internal.h b/include/qapi/error-internal.h
> index d5c3904adec..6178ce4a63d 100644
> --- a/include/qapi/error-internal.h
> +++ b/include/qapi/error-internal.h
> @@ -19,6 +19,7 @@ struct Error
> char *msg;
> ErrorClass err_class;
> const char *src, *func;
> +ssize_t src_len;
> int line;
> GString *hint;
> };
> diff --git a/util/error.c b/util/error.c
> index e5bcb7c0225..6c1033eaba5 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -24,8 +24,13 @@ Error *error_warn;
> static void error_handle(Error **errp, Error *err)
> {
> if (errp == &error_abort) {
> +const char *src = err->src;
> +if (err->src_len >= 0) {
> +/* No need to free it, the program will abort very soon... */
> +src = g_strndup(err->src, err->src_len);
> +}
> fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> -err->func, err->src, err->line);
> +err->func, src, err->line);
... you're avoiding the simpler
fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
err->func, err->src_len, err->src, err->line);
because of it.
(@src_len needs to be int then, and its default value below INT_MAX)
> error_report("%s", error_get_pretty(err));
> if (err->hint) {
> error_printf("%s", err->hint->str);
> @@ -67,6 +72,7 @@ static void error_setv(Error **errp,
> g_free(msg);
> }
> err->err_class = err_class;
> +err->src_len = -1;
> err->src = src;
> err->line = line;
> err->func = func;
