Re: g_autoptr(Error)

["Cédric Le Goater" <clg@redhat.com>]

On 11/25/25 17:15, Peter Xu wrote:
> On Tue, Nov 25, 2025 at 12:46:01PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Tue, Nov 25, 2025 at 08:40:07AM +0100, Markus Armbruster wrote:
>>>> g_autoptr(T) is quite useful when the object's extent matches the
>>>> function's.
>>>>
>>>> This isn't the case for an Error object the function propagates to its
>>>> caller.  It is the case for an Error object the function reports or
>>>> handles itself.  However, the functions to report Error also free it.
> 
> I'd confess I didn't pay enough attention on how the error API was designed
> deliberately to always free the Error objects before almost whenever
> possible.  But I see now, thanks for the write up.
> 
>>>>
>>>> Thus, g_autoptr(Error) is rarely applicable.  We have just three
>>>> instances out of >1100 local Error variables, all in migration code.
>>>>
>>>> Two want to move the error to the MigrationState for later handling /
>>>> reporting.  Since migrate_set_error() doesn't move, but stores a copy,
>>>> the original needs to be freed, and g_autoptr() is correct there.  We
>>>> have 17 more that instead manually free with error_free() or
>>>> error_report_err() right after migrate_set_error().
>>>>
>>>> We recently discussed storing a copy vs. move the original:
>>>>
>>>>      From: Peter Xu <peterx@redhat.com>
>>>>      Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>>>>      Date: Mon, 17 Nov 2025 11:03:37 -0500
>>>>      Message-ID: <aRtHWbWcTh3OF2wY@x1.local>
>>>>
>>>> The two g_autoptr() gave me pause when I investigated this topic, simply
>>>> because they deviate from the common pattern migrate_set_error(s, err)
>>>> followed by error_free() or error_report_err().
>>>>
>>>> The third one became wrong when I cleaned up the reporting (missed in
>>>> the cleanup patch, fixed in the patch I'm replying to).  I suspect my
>>>> mistake escaped review for the same reason I made it: g_autoptr(Error)
>>>> is unusual and not visible in the patch hunk.
>>>>
>>>> Would you like me to replace the two correct uses of g_autoptr(Error) by
>>>> more common usage?
> 
> Works for me.
> 
> Now I also think it should be good migrate_set_error() follow QEMU's Error
> API design if we decide to stick with it freeing errors in such APIs.
> 
> Said that, I wonder if you think we could still consider passing Error**
> into migrate_set_error(), though, which will be a merged solution of
> current Error API and what Marc-Andre proposed on resetting pointers to
> avoid any possible UAF, which I would still slightly prefer personally.
> 
> If we rework migrate_set_error() to take ownership first, then we can
> naturally drop the two use cases, and remove the cleanup function.
> 
> Markus, please also let me know if you want me to do it.
> 
>>>
>>> I had previously proposed g_autoptr(Error) a year or two back and you
>>> rejected it then, so I'm surprised to see that it got into the code,
>>> because it requires explicit opt-in via a G_DEFINE_AUTOPTR_CLEANUP_FUNC.
>>>
>>> Unfortunately it appears exactly that was added earlier this year in
>>>
>>>    commit 18eb55546a54e443d94a4c49286348176ad4b00a
>>>    Author: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>    Date:   Tue Mar 4 23:03:35 2025 +0100
>>>
>>>      error: define g_autoptr() cleanup function for the Error type
>>>      
>>>      Automatic memory management helps avoid memory safety issues.
>>>      
>>>      Reviewed-by: Peter Xu <peterx@redhat.com>
>>>      Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>      Link: https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigiero@oracle.com
>>>      Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>
>> I missed it.  Not he submitter's fault; it was cc'ed to me.
> 
> If someone to blame, it's the reviewer.
At end, I was the one who merged this stuff. My bad.

I felt confident at the time, as it was only a single-line change reviewed
by a subsystem maintainer and the patch was large enough that this didn't
raise my attention.

But it should have been treated with greater caution, global features must
be introduced together with concrete usage proposals. I think this would
have raised some unconscious red flags.

Thanks,

C.