Thank you Matthew for the quick response and I see you already added
Frode who was driving this change.

I have reviewed the PPA and the diff that it has right now.

Yes 77401d54 seems present and c0ae4919e would be needed for that as dependency.
The code still has the needed VIR_DIR_CLOSE so it is not also leaking.

Around firstEntryName where the current issue happens the formerly merged 
refactor had:
  *netname = g_steal_pointer(&firstEntryName);
But the backport was
  *netname = firstEntryName;
And now your proposed solution is to add:
  firstEntryName = NULL;

(all that on a g_autofree type)


Let us play this through:


So in the upstream code it does:
  firstEntryName = NULL;     (init)
  firstEntryName = g_strdup(entry->d_name);     (assign a value)
  *netname = g_steal_pointer(&firstEntryName);  (transfer ownership, sets NULL)
  on return g_autofree frees firstEntryName     (being NULL this is a no-op, 
but a fallback for 
    other return paths)

In the current bad backport it does:
  firstEntryName = NULL; (init)
  firstEntryName = g_strdup(entry->d_name);     (assign a value)
  *netname = firstEntryName;                    (direct take over, leaving 
firstEntryName as-is)
  on return g_autofree frees firstEntryName which still being used from netname 
causes the crash

The suggestion to set
  firstEntryName = NULL;
will prevent the auto-clean from freeing the value and thereby prevent the 
segfault.
But will there be any return paths where it would need to be freed because of a 
leak?
This happens late on the way out which is good.
Yeah - I agree there is no return path that would leak firstEntryName.
But for clarity and matching what eventually is upstream is there a reason not 
to use g_steal_pointer in the very same place. The includes are already in 
place other code in virpci.c already uses it.

So instead of
  *netname = firstEntryName;                                           
  firstEntryName = NULL;
maybe
  *netname = g_steal_pointer(&firstEntryName);

Both should be fine, not stopping you.
I've also taken the chance to reference this bug in the "testcase that should 
be added to regression tests" list.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1943481

Title:
  libvirtd crashes when creating network interface pools in
  6.0.0-0ubuntu8.13

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1943481/+subscriptions


-- 
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to