1. Patchew
  2. QEMU
  3. [PATCH 0/3] migration: A few fixes on coverity reports
  4. View patch

[PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

Peter Xu posted 3 patches 3 weeks, 3 days ago
Maintainers: Steve Sistare <steven.sistare@oracle.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
There is a newer version of this series
[PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()
Posted by Peter Xu 3 weeks, 3 days ago 
Per reported and analyzed by Peter:

https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com

mfd leak is a false positive, try to use a coverity annotation (which I
didn't find manual myself, but still give it a shot).

Fix the other one by dumping an error message if setenv() failed.

Resolves: Coverity CID 1641391
Resolves: Coverity CID 1641392
Fixes: efc6587313 ("migration: cpr-exec save and load")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/cpr-exec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index d57714bc5d..3cf44634a9 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
 void cpr_exec_persist_state(QEMUFile *f)
 {
     QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
+    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
     int mfd = dup(fioc->fd);
     char val[16];
 
     /* Remember mfd in environment for post-exec load */
     qemu_clear_cloexec(mfd);
     snprintf(val, sizeof(val), "%d", mfd);
-    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
+    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
+        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
+    }
 }
 
 static int cpr_exec_find_state(void)
-- 
2.50.1
Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()
Posted by Fabiano Rosas 3 weeks, 2 days ago 
Peter Xu <peterx@redhat.com> writes:

> Per reported and analyzed by Peter:
>
> https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com
>
> mfd leak is a false positive, try to use a coverity annotation (which I
> didn't find manual myself, but still give it a shot).
>
> Fix the other one by dumping an error message if setenv() failed.
>
> Resolves: Coverity CID 1641391
> Resolves: Coverity CID 1641392
> Fixes: efc6587313 ("migration: cpr-exec save and load")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/cpr-exec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> index d57714bc5d..3cf44634a9 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
> @@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
>  void cpr_exec_persist_state(QEMUFile *f)
>  {
>      QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
> +    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
>      int mfd = dup(fioc->fd);
>      char val[16];
>  
>      /* Remember mfd in environment for post-exec load */
>      qemu_clear_cloexec(mfd);
>      snprintf(val, sizeof(val), "%d", mfd);
> -    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
> +    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
> +        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
> +    }

Best to abort no? We don't want the rest of the code reading whatever
may be at that env variable and running with it.

>  }
>  
>  static int cpr_exec_find_state(void)
Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()
Posted by Peter Xu 3 weeks, 2 days ago 
On Tue, Oct 21, 2025 at 05:49:07PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Per reported and analyzed by Peter:
> >
> > https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com
> >
> > mfd leak is a false positive, try to use a coverity annotation (which I
> > didn't find manual myself, but still give it a shot).
> >
> > Fix the other one by dumping an error message if setenv() failed.
> >
> > Resolves: Coverity CID 1641391
> > Resolves: Coverity CID 1641392
> > Fixes: efc6587313 ("migration: cpr-exec save and load")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/cpr-exec.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> > index d57714bc5d..3cf44634a9 100644
> > --- a/migration/cpr-exec.c
> > +++ b/migration/cpr-exec.c
> > @@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
> >  void cpr_exec_persist_state(QEMUFile *f)
> >  {
> >      QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
> > +    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
> >      int mfd = dup(fioc->fd);
> >      char val[16];
> >  
> >      /* Remember mfd in environment for post-exec load */
> >      qemu_clear_cloexec(mfd);
> >      snprintf(val, sizeof(val), "%d", mfd);
> > -    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
> > +    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
> > +        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
> > +    }
> 
> Best to abort no? We don't want the rest of the code reading whatever
> may be at that env variable and running with it.

I didn't want to abort, because it's the same as QMP migrate failure at the
beginning.

If we want to do better, we can allow this function to return a failure
instead.

And.. when I was trying cpr-exec with no argv it already can crash QEMU.  I
think that's also a bug.

Let me prepare something better than this..  I'll likely add a new patch to
fix that too.

> 
> >  }
> >  
> >  static int cpr_exec_find_state(void)
> 

-- 
Peter Xu
Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()
Posted by Fabiano Rosas 3 weeks, 2 days ago 
Peter Xu <peterx@redhat.com> writes:

> On Tue, Oct 21, 2025 at 05:49:07PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Per reported and analyzed by Peter:
>> >
>> > https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com
>> >
>> > mfd leak is a false positive, try to use a coverity annotation (which I
>> > didn't find manual myself, but still give it a shot).
>> >
>> > Fix the other one by dumping an error message if setenv() failed.
>> >
>> > Resolves: Coverity CID 1641391
>> > Resolves: Coverity CID 1641392
>> > Fixes: efc6587313 ("migration: cpr-exec save and load")
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/cpr-exec.c | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
>> > index d57714bc5d..3cf44634a9 100644
>> > --- a/migration/cpr-exec.c
>> > +++ b/migration/cpr-exec.c
>> > @@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
>> >  void cpr_exec_persist_state(QEMUFile *f)
>> >  {
>> >      QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
>> > +    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
>> >      int mfd = dup(fioc->fd);
>> >      char val[16];
>> >  
>> >      /* Remember mfd in environment for post-exec load */
>> >      qemu_clear_cloexec(mfd);
>> >      snprintf(val, sizeof(val), "%d", mfd);
>> > -    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
>> > +    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
>> > +        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
>> > +    }
>> 
>> Best to abort no? We don't want the rest of the code reading whatever
>> may be at that env variable and running with it.
>
> I didn't want to abort, because it's the same as QMP migrate failure at the
> beginning.
>
> If we want to do better, we can allow this function to return a failure
> instead.
>
> And.. when I was trying cpr-exec with no argv it already can crash QEMU.  I
> think that's also a bug.
>
> Let me prepare something better than this..  I'll likely add a new patch to
> fix that too.
>

Remember to copy Oracle folks.

>> 
>> >  }
>> >  
>> >  static int cpr_exec_find_state(void)
>>
Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()
Posted by Peter Xu 3 weeks, 2 days ago 
On Wed, Oct 22, 2025 at 09:24:25AM -0300, Fabiano Rosas wrote:
> Remember to copy Oracle folks.

Do you know who's taking this over from Steve?  If we know, we should
update the MAINTAINERS file.

-- 
Peter Xu
Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()
Posted by Fabiano Rosas 3 weeks, 2 days ago 
Peter Xu <peterx@redhat.com> writes:

> On Wed, Oct 22, 2025 at 09:24:25AM -0300, Fabiano Rosas wrote:
>> Remember to copy Oracle folks.
>
> Do you know who's taking this over from Steve?  If we know, we should
> update the MAINTAINERS file.

I'm not sure about maintainership but I'd be already glad to get testing
and reviews. Steve mentioned Mark Kanda on his email. I'm CCing Mark
here and he can tell us about it. =) No pressure, just making sure CPR
work is not falling between the cracks really.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.