[PATCH] qemu-pr-helper: do not free reply when multipath_process_cmd() fails

[PATCH] qemu-pr-helper: do not free reply when multipath_process_cmd() fails

[Stefan Hajnoczi]

Kevin Wolf pointed out that the multipath_process_cmd() documentation
only defines the value of reply on success. Don't touch it when the
function returns -1.

The libmpathcmd implementation as of c78cf64a9e8e does not set *reply to
NULL when its internal mpath_send_cmd() call fails, so I think Kevin's
interpretation is correct.

As it stands, there is no observable bug in the QEMU code though because
reply is initialized to NULL before calling multipath_process_cmd().
This patch is just a cleanup.

Cc: Benjamin Marzinski <bmarzins@redhat.com>
Cc: kwolf@redhat.com
Cc: eblake@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Eric pointed out that g_autofree could eliminate the manual free(3)
calls. In this case we cannot use it since g_autofree calls glib's
g_free() rather than the standard library's free(3).
---
 scsi/qemu-pr-helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 54d196a503f..d52109cab25 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -571,13 +571,17 @@ static bool multipathd_cmd(int fd, const char *cmd, Error **errp)
         free(reply);
         return false;
     }
-    free(reply);
     if (ret == -1) {
+        /*
+         * reply is not freed because the documentation only defines it when
+         * the function returns success.
+         */
         error_setg_errno(errp, last_errno,
                          "mpath_process_cmd(%d, \"%s\") failed",
                          fd, cmd);
         return false;
     }
+    free(reply);
     return true;
 }
 
-- 
2.56.0

Re: [PATCH] qemu-pr-helper: do not free reply when multipath_process_cmd() fails

[Eric Blake]

On Mon, Oct 19, 2026 at 11:33:53AM -0400, Stefan Hajnoczi wrote:
> Kevin Wolf pointed out that the multipath_process_cmd() documentation
> only defines the value of reply on success. Don't touch it when the
> function returns -1.
> 
> The libmpathcmd implementation as of c78cf64a9e8e does not set *reply to
> NULL when its internal mpath_send_cmd() call fails, so I think Kevin's
> interpretation is correct.
> 
> As it stands, there is no observable bug in the QEMU code though because
> reply is initialized to NULL before calling multipath_process_cmd().
> This patch is just a cleanup.
> 
> Cc: Benjamin Marzinski <bmarzins@redhat.com>
> Cc: kwolf@redhat.com
> Cc: eblake@redhat.com
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Eric pointed out that g_autofree could eliminate the manual free(3)
> calls. In this case we cannot use it since g_autofree calls glib's
> g_free() rather than the standard library's free(3).

I know glib encourages g_free() in case malloc was interposed, but
aren't we guaranteed that we are not exploiting glib's malloc
overrides, and therefore g_free() calls into free(3)?  But I'm fine
with your decision to not use g_autofree here.

> ---
>  scsi/qemu-pr-helper.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 54d196a503f..d52109cab25 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -571,13 +571,17 @@ static bool multipathd_cmd(int fd, const char *cmd, Error **errp)
>          free(reply);
>          return false;
>      }
> -    free(reply);
>      if (ret == -1) {
> +        /*
> +         * reply is not freed because the documentation only defines it when
> +         * the function returns success.
> +         */
>          error_setg_errno(errp, last_errno,
>                           "mpath_process_cmd(%d, \"%s\") failed",
>                           fd, cmd);
>          return false;
>      }
> +    free(reply);

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] qemu-pr-helper: do not free reply when multipath_process_cmd() fails

[Stefan Hajnoczi]

On Mon, Oct 19, 2026 at 11:19:29AM -0500, Eric Blake wrote:
> On Mon, Oct 19, 2026 at 11:33:53AM -0400, Stefan Hajnoczi wrote:
> > Kevin Wolf pointed out that the multipath_process_cmd() documentation
> > only defines the value of reply on success. Don't touch it when the
> > function returns -1.
> > 
> > The libmpathcmd implementation as of c78cf64a9e8e does not set *reply to
> > NULL when its internal mpath_send_cmd() call fails, so I think Kevin's
> > interpretation is correct.
> > 
> > As it stands, there is no observable bug in the QEMU code though because
> > reply is initialized to NULL before calling multipath_process_cmd().
> > This patch is just a cleanup.
> > 
> > Cc: Benjamin Marzinski <bmarzins@redhat.com>
> > Cc: kwolf@redhat.com
> > Cc: eblake@redhat.com
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > Eric pointed out that g_autofree could eliminate the manual free(3)
> > calls. In this case we cannot use it since g_autofree calls glib's
> > g_free() rather than the standard library's free(3).
> 
> I know glib encourages g_free() in case malloc was interposed, but
> aren't we guaranteed that we are not exploiting glib's malloc
> overrides, and therefore g_free() calls into free(3)?  But I'm fine
> with your decision to not use g_autofree here.

g_mem_set_vtable() is deprecated and a nop. Interposing no longer works.
g_mem_is_system_malloc() always returns TRUE.

If we take advantage of glib internals, then you can call g_free() on
malloc() memory AFAIK. But I don't think the API documentation
guarantees this will always be the case and I'm not sure it's worth
baking in this assumption.

Maybe there is a clean way to do this with the glib API: define a type
called SystemMallocPtr and then use g_autoptr(SystemMallocPtr)?

If someone with more glib knowledge has a clean solution, we could start
using it. IMO if you need to think too much about it, it's probably best
to do the simple thing instead.

> 
> > ---
> >  scsi/qemu-pr-helper.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> > index 54d196a503f..d52109cab25 100644
> > --- a/scsi/qemu-pr-helper.c
> > +++ b/scsi/qemu-pr-helper.c
> > @@ -571,13 +571,17 @@ static bool multipathd_cmd(int fd, const char *cmd, Error **errp)
> >          free(reply);
> >          return false;
> >      }
> > -    free(reply);
> >      if (ret == -1) {
> > +        /*
> > +         * reply is not freed because the documentation only defines it when
> > +         * the function returns success.
> > +         */
> >          error_setg_errno(errp, last_errno,
> >                           "mpath_process_cmd(%d, \"%s\") failed",
> >                           fd, cmd);
> >          return false;
> >      }
> > +    free(reply);
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org
>