On 11/26/25 09:19, Cédric Le Goater wrote:
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é <[email protected]> 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 <[email protected]>
Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
Date: Mon, 17 Nov 2025 11:03:37 -0500
Message-ID: <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Maciej S. Szmigiero <[email protected]>
Link:
https://lore.kernel.org/qemu-devel/a5843c5fa64d7e5239a4316092ec0ef0d10c2320.1741124640.git.maciej.szmigi...@oracle.com
Signed-off-by: Cédric Le Goater <[email protected]>
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
s/patch/series/ makes more sense.
Sorry for the noise.
C.
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.
