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 <[email protected]>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3905
> > Signed-off-by: Maxim Levitsky <[email protected]>
> > ---
> > 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
