system/runstate.c | 35 +++++++---------------------------- 1 file changed, 7 insertions(+), 28 deletions(-)
tdx_parse_panic_message has code that detects if the guest supplied
message contains some unprintable characters and in this case prints a hex dump.
The code has a bug: it allocates 3 bytes for 'sprintf("%02x ")',
but sprintf in this case actually writes 4 bytes, because it adds the NULL
terminator.
To fix this, just use qemu_hexdump_line instead.
This patch was only compile tested.
Reported-by: Jordy Zomer <jordyzomer@google.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3905
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
system/runstate.c | 35 +++++++----------------------------
1 file changed, 7 insertions(+), 28 deletions(-)
diff --git a/system/runstate.c b/system/runstate.c
index d3e64d2b6258..ebf09b192b4e 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -50,6 +50,7 @@
#include "qemu/sockets.h"
#include "qemu/timer.h"
#include "qemu/thread.h"
+#include "qemu/cutils.h"
#include "qom/object.h"
#include "qom/object_interfaces.h"
#include "system/cpu-timers.h"
@@ -760,10 +761,9 @@ static void qemu_system_wakeup(void)
}
}
-static char *tdx_parse_panic_message(char *message)
+static GString *tdx_parse_panic_message(char *message)
{
bool printable = false;
- char *buf = NULL;
int len = 0, i;
/*
@@ -784,32 +784,11 @@ static char *tdx_parse_panic_message(char *message)
}
}
- if (len == 0) {
- buf = g_malloc(1);
- buf[0] = '\0';
+ if (printable) {
+ return g_string_new(message);
} else {
- if (!printable) {
- /* 3 = length of "%02x " */
- buf = g_malloc(len * 3);
- for (i = 0; i < len; i++) {
- if (message[i] == '\0') {
- break;
- } else {
- sprintf(buf + 3 * i, "%02x ", message[i]);
- }
- }
- if (i > 0) {
- /* replace the last ' '(space) to NULL */
- buf[i * 3 - 1] = '\0';
- } else {
- buf[0] = '\0';
- }
- } else {
- buf = g_strdup(message);
- }
+ return qemu_hexdump_line(NULL, message, len, 1, 0);
}
-
- return buf;
}
void qemu_system_guest_panicked(GuestPanicInformation *info)
@@ -854,11 +833,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
info->u.s390.psw_mask,
info->u.s390.psw_addr);
} else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) {
- char *message = tdx_parse_panic_message(info->u.tdx.message);
+ GString *message = tdx_parse_panic_message(info->u.tdx.message);
qemu_log_mask(LOG_GUEST_ERROR,
"\nTDX guest reports fatal error."
" error code: 0x%" PRIx32 " error message:\"%s\"\n",
- info->u.tdx.error_code, message);
+ info->u.tdx.error_code, message->str);
g_free(message);
if (info->u.tdx.has_gpa) {
qemu_log_mask(LOG_GUEST_ERROR, "Additional error information "
--
2.54.0On 9/23/26 02:45, Maxim Levitsky wrote:
> tdx_parse_panic_message has code that detects if the guest supplied
> message contains some unprintable characters and in this case prints a hex dump.
>
> The code has a bug: it allocates 3 bytes for 'sprintf("%02x ")',
> but sprintf in this case actually writes 4 bytes, because it adds the NULL
> terminator.
>
> To fix this, just use qemu_hexdump_line instead.
> This patch was only compile tested.
>
> Reported-by: Jordy Zomer <jordyzomer@google.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3905
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
> system/runstate.c | 35 +++++++----------------------------
> 1 file changed, 7 insertions(+), 28 deletions(-)
>
> diff --git a/system/runstate.c b/system/runstate.c
> index d3e64d2b6258..ebf09b192b4e 100644
> --- a/system/runstate.c
> +++ b/system/runstate.c
> @@ -50,6 +50,7 @@
> #include "qemu/sockets.h"
> #include "qemu/timer.h"
> #include "qemu/thread.h"
> +#include "qemu/cutils.h"
> #include "qom/object.h"
> #include "qom/object_interfaces.h"
> #include "system/cpu-timers.h"
> @@ -760,10 +761,9 @@ static void qemu_system_wakeup(void)
> }
> }
>
> -static char *tdx_parse_panic_message(char *message)
> +static GString *tdx_parse_panic_message(char *message)
> {
> bool printable = false;
> - char *buf = NULL;
> int len = 0, i;
>
> /*
> @@ -784,32 +784,11 @@ static char *tdx_parse_panic_message(char *message)
> }
> }
>
> - if (len == 0) {
> - buf = g_malloc(1);
> - buf[0] = '\0';
> + if (printable) {
> + return g_string_new(message);
> } else {
> - if (!printable) {
> - /* 3 = length of "%02x " */
> - buf = g_malloc(len * 3);
> - for (i = 0; i < len; i++) {
> - if (message[i] == '\0') {
> - break;
> - } else {
> - sprintf(buf + 3 * i, "%02x ", message[i]);
> - }
> - }
> - if (i > 0) {
> - /* replace the last ' '(space) to NULL */
> - buf[i * 3 - 1] = '\0';
> - } else {
> - buf[0] = '\0';
> - }
> - } else {
> - buf = g_strdup(message);
> - }
> + return qemu_hexdump_line(NULL, message, len, 1, 0);
> }
> -
> - return buf;
> }
>
> void qemu_system_guest_panicked(GuestPanicInformation *info)
> @@ -854,11 +833,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
> info->u.s390.psw_mask,
> info->u.s390.psw_addr);
> } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) {
> - char *message = tdx_parse_panic_message(info->u.tdx.message);
> + GString *message = tdx_parse_panic_message(info->u.tdx.message);
> qemu_log_mask(LOG_GUEST_ERROR,
> "\nTDX guest reports fatal error."
> " error code: 0x%" PRIx32 " error message:\"%s\"\n",
> - info->u.tdx.error_code, message);
> + info->u.tdx.error_code, message->str);
> g_free(message);
Freeing the GString* leaks message->str.
The easiest way to do it, which I have squashed on top of your patch, is
g_autoptr(GString) message = ...
and dropping the g_free(message) line. See for example sdbus_write_dump().
Paolo
On Wed, 2026-09-23 at 16:36 +0200, Paolo Bonzini wrote:
> On 9/23/26 02:45, Maxim Levitsky wrote:
> > tdx_parse_panic_message has code that detects if the guest supplied
> > message contains some unprintable characters and in this case prints a hex dump.
> >
> > The code has a bug: it allocates 3 bytes for 'sprintf("%02x ")',
> > but sprintf in this case actually writes 4 bytes, because it adds the NULL
> > terminator.
> >
> > To fix this, just use qemu_hexdump_line instead.
> > This patch was only compile tested.
> >
> > Reported-by: Jordy Zomer <jordyzomer@google.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3905
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > system/runstate.c | 35 +++++++----------------------------
> > 1 file changed, 7 insertions(+), 28 deletions(-)
> >
> > diff --git a/system/runstate.c b/system/runstate.c
> > index d3e64d2b6258..ebf09b192b4e 100644
> > --- a/system/runstate.c
> > +++ b/system/runstate.c
> > @@ -50,6 +50,7 @@
> > #include "qemu/sockets.h"
> > #include "qemu/timer.h"
> > #include "qemu/thread.h"
> > +#include "qemu/cutils.h"
> > #include "qom/object.h"
> > #include "qom/object_interfaces.h"
> > #include "system/cpu-timers.h"
> > @@ -760,10 +761,9 @@ static void qemu_system_wakeup(void)
> > }
> > }
> >
> > -static char *tdx_parse_panic_message(char *message)
> > +static GString *tdx_parse_panic_message(char *message)
> > {
> > bool printable = false;
> > - char *buf = NULL;
> > int len = 0, i;
> >
> > /*
> > @@ -784,32 +784,11 @@ static char *tdx_parse_panic_message(char *message)
> > }
> > }
> >
> > - if (len == 0) {
> > - buf = g_malloc(1);
> > - buf[0] = '\0';
> > + if (printable) {
> > + return g_string_new(message);
> > } else {
> > - if (!printable) {
> > - /* 3 = length of "%02x " */
> > - buf = g_malloc(len * 3);
> > - for (i = 0; i < len; i++) {
> > - if (message[i] == '\0') {
> > - break;
> > - } else {
> > - sprintf(buf + 3 * i, "%02x ", message[i]);
> > - }
> > - }
> > - if (i > 0) {
> > - /* replace the last ' '(space) to NULL */
> > - buf[i * 3 - 1] = '\0';
> > - } else {
> > - buf[0] = '\0';
> > - }
> > - } else {
> > - buf = g_strdup(message);
> > - }
> > + return qemu_hexdump_line(NULL, message, len, 1, 0);
> > }
> > -
> > - return buf;
> > }
> >
> > void qemu_system_guest_panicked(GuestPanicInformation *info)
> > @@ -854,11 +833,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
> > info->u.s390.psw_mask,
> > info->u.s390.psw_addr);
> > } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) {
> > - char *message = tdx_parse_panic_message(info->u.tdx.message);
> > + GString *message = tdx_parse_panic_message(info->u.tdx.message);
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "\nTDX guest reports fatal error."
> > " error code: 0x%" PRIx32 " error message:\"%s\"\n",
> > - info->u.tdx.error_code, message);
> > + info->u.tdx.error_code, message->str);
> > g_free(message);
>
> Freeing the GString* leaks message->str.
>
> The easiest way to do it, which I have squashed on top of your patch, is
>
> g_autoptr(GString) message = ...
>
> and dropping the g_free(message) line. See for example sdbus_write_dump().
Oops, thanks for fixing this, I thought about that g_free(), but I guess not enough.
Best regards,
Maxim Levitsky
>
> Paolo
© 2016 - 2026 Red Hat, Inc.