]> git.ipfire.org Git - thirdparty/libvirt.git/commit
lxc: eliminate leaked and dangling pointers in virLXCProcessSetupInterfaceTap
authorLaine Stump <laine@redhat.com>
Wed, 6 Jan 2021 18:44:40 +0000 (13:44 -0500)
committerLaine Stump <laine@redhat.com>
Fri, 8 Jan 2021 00:41:27 +0000 (19:41 -0500)
commit2d0bac9d58775ef50c76beedfb9b4a4f80af1d55
tree324648169fba00e2cd28d579498cafc68bfc72f1
parent84617bf2f8d3e83a078414b92bc1d8a94c79269b
lxc: eliminate leaked and dangling pointers in virLXCProcessSetupInterfaceTap

The two scenarios were found by Coverity after a seemingly-unrelated
change to virLXCProcessSetupInterfaceTap() (in commit ecfc2d5f43), and
explained by John Ferlan here:

https://www.redhat.com/archives/libvir-list/2020-December/msg00810.html

To re-explain:

a) On entry to virLXCProcessSetupInterfaceTap() if net->ifname != NULL
   then a copy of net->ifname is made into parentVeth, and a reference
   to *that* pointer is sent down to virNetDevVethCreate().

b) If parentVeth (aka net->ifname) is a template name (e.g. "blah%d"),
   then virNetDevVethCreate() calls virNetDevGenerateName(), and if
   virNetDevGenerateName() successfully generates a usable name
   (e.g. "blah27") then it will free the original template string
   (which is pointed to by net->ifname and by parentVeth), then
   replace the pointer in parentVeth with a pointer to the new
   string. Note that net->ifname still points to the now-freed
   template string.

c) returning back up to virLXCProcessSetupInterfaceTap(), we check if
   net->ifname == NULL - it *isn't* (still contains stale pointer to
   template string), so we don't replace it with the pointer to the new
   string that is in parentVeth.

d) Result: the new string is leaked once we return from
   virLXCProcessSetupInterfaceTap(), while there is a dangling pointer
   to the old string in net->ifname.

There is also a leak if there is a failure somewhere between steps (b)
and (c) above - the failure cleanup in virNetDevVethCreate() will only
free the newly-generated parentVeth string if the original pointer was
NULL (narrator: "It wasn't."). But it's a new string allocated by
virNetDevGenerateName(), not the original string from net->ifname, so
it really does need to be freed.

The solution is to make a copy of the entire original string into a
g_autofree pointer, then iff everything is successful we g_free() the
original net->ifname and replace it by stealing the string returned by
virNetDevVethCreate().

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
src/lxc/lxc_process.c