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

[Fabiano Rosas <farosas@suse.de>]

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)
>>