On Wed, Feb 25, 2026 at 12:43:38PM +0100, Stefan Kober wrote: > We have a g_autoptr ret in the virIdentityGetSystem function. In the > happy path it is properly returned by doing: return g_steal_pointer(&ret); > > There are 2 early return paths, were we do the following: "return ret;" > > This leads to the g_autoptr being cleaned up after we leave the > function, as we do not properly "steal" it. > > When later using the return value we have a use-after-free, which has > led to segfaults in our case. > > We fix the early returns by doing the same as in the happy path.
Actually the flaw here is that we should *NOT* have early returns that use 'ret' at all. An identity must only be returned if *all* data is fetched successfully. This is a regression accidentally introduced in commit 1280a631ef488aeaab905eb30a55899ef8ba97be Author: Daniel P. Berrangé <[email protected]> Date: Thu Aug 5 19:03:19 2021 +0100 src: stop checking virIdentityNew return value where I moved the 'virIdentityNew' call. Previously 'ret' would still be NULL in these places where we 'return ret', so it was effectively 'return NULL'. When I moved 'virIdentityNew' I broke this equivalence. So the fix here is to change 'return ret' to 'return NULL' > > On-behalf-of: SAP [email protected] > Signed-off-by: Stefan Kober <[email protected]> > --- > src/util/viridentity.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/util/viridentity.c b/src/util/viridentity.c > index b7b88056ac..10935fba60 100644 > --- a/src/util/viridentity.c > +++ b/src/util/viridentity.c > @@ -327,15 +327,19 @@ virIdentity *virIdentityGetSystem(void) > virIdentitySetProcessTime(ret, startTime) < 0) > return NULL; > > - if (!(username = virGetUserName(geteuid()))) > - return ret; > + if (!(username = virGetUserName(geteuid()))) { > + VIR_WARN("virGetUserName failed, returning partial identity"); > + return g_steal_pointer(&ret); > + } > if (virIdentitySetUserName(ret, username) < 0) > return NULL; > if (virIdentitySetUNIXUserID(ret, getuid()) < 0) > return NULL; > > - if (!(groupname = virGetGroupName(getegid()))) > - return ret; > + if (!(groupname = virGetGroupName(getegid()))) { > + VIR_WARN("virGetGroupName failed, returning partial identity"); > + return g_steal_pointer(&ret); > + } > if (virIdentitySetGroupName(ret, groupname) < 0) > return NULL; > if (virIdentitySetUNIXGroupID(ret, getgid()) < 0) > -- > 2.53.0 > With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
