Comment 9 for bug 1943481

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

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.