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.

On-behalf-of: SAP stefan.kober@sap.com
Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de>
---
 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

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é <berrange@redhat.com>
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 stefan.kober@sap.com
> Signed-off-by: Stefan Kober <stefan.kober@cyberus-technology.de>
> ---
>  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 :|

Thanks for the quick response!

I wasn't sure about the desired semantics but it looked like we deliberately allow to have these partial identities.

I will prepare an updated version that returns Null in those cases.