vircgroup: Don't leak @parent in virCgroupEnableMissingControllers()
A memory leak was identified in
virCgroupEnableMissingControllers():
==11680== at 0x483EAE5: calloc (vg_replace_malloc.c:760)
==11680== by 0x4E51780: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6701.0)
==11680== by 0x4908618: virCgroupNew (vircgroup.c:701)
==11680== by 0x49096F4: virCgroupEnableMissingControllers (vircgroup.c:1146)
==11680== by 0x4909B17: virCgroupNewMachineSystemd (vircgroup.c:1228)
==11680== by 0x4909E94: virCgroupNewMachine (vircgroup.c:1313)
==11680== by 0x1694FDBC: qemuInitCgroup (qemu_cgroup.c:946)
==11680== by 0x1695046B: qemuSetupCgroup (qemu_cgroup.c:1083)
==11680== by 0x16A60126: qemuProcessLaunch (qemu_process.c:7077)
==11680== by 0x16A61504: qemuProcessStart (qemu_process.c:7384)
==11680== by 0x169B84C2: qemuDomainObjStart (qemu_driver.c:6590)
==11680== by 0x169B8776: qemuDomainCreateWithFlags (qemu_driver.c:6641)
What happens is that new virCgroup is created and stored into
@parent. Then, if @tokens is not empty the for() loop is entered
into where another virCgroup is created and @parent is replaced
with this new virCgroup. But nothing freed the old @parent.
Fixes: 77291414c7a8745cf4d2b06d3c38d269cfbcfe32 Reported-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Laine Stump [Tue, 2 Feb 2021 17:51:48 +0000 (12:51 -0500)]
build: fix specfile logic for disabling netcf
I *thought* I had tested all the combinations of manually setting
--without netcf, different versions of Fedora, etc, but apparently
not.
The check in libvirt.spec.in to see if the target was an older Fedora
or older RHEL would alway resolve to true, because, e.g., if {?fedora}
is undefined, then "0%{?fedora} < 34" is "0 < 34", which is always
true. Since both {?fedora} and {?rhel} are never defined at the same
time, the result of the entire expression is always true.
Fix this by qualifying each subexpression.
Fixes: 35d5b26aa433bd33f4b33be3dbb67313357f97f9 Signed-off-by: Laine Stump <laine@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Krempa [Wed, 3 Feb 2021 12:43:28 +0000 (13:43 +0100)]
viralloc: Remove VIR_ALLOC_VAR
The use case VIR_ALLOC_VAR deals with is very unlikely. We had just 2
legitimate uses, which were reimplemented locally using g_malloc0 and
sizeof instead as they used a static number of members of the trailing
array.
Remove VIR_ALLOC_VAR since in most cases the direct implementation is
shorter and clearer and there are no users of it currently.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Krempa [Tue, 2 Feb 2021 16:04:30 +0000 (17:04 +0100)]
qemuBuildRBDSecinfoURI: Use virSecureEraseString instead of VIR_AUTODISPOSE_STR
In this instance attempting to be correct is really pointless since the
secret is formatted into another string which is not erased securely and
then put on the commandline.
Keep the secure handling for correctness.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Krempa [Mon, 1 Feb 2021 13:16:54 +0000 (14:16 +0100)]
tests: viralloc: Remove testDispose case
The VIR_DISPOSE* APIs will be phased out. Additionally the test isn't
really doing useful work in ensuring that the values are indeed cleared
thus there's no point in keeping it around.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The code pretends that it cares about clearing the secret values, but
passes the secret value to a realloc, which may copy the value somewhere
else and doesn't sanitize the original location when it does so.
Since we want to construct a string from the value, let's copy it to a
new piece of memory which has the space for the 'NUL' byte ourselves, to
prevent a random realloc keeping the data around.
While at it, use virSecureErase instead of VIR_DISPOSE_N since it's
being phased out.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Krempa [Mon, 1 Feb 2021 13:01:57 +0000 (14:01 +0100)]
virsh: cmdSecretSetValue: Rework handling of the secret value
Use a single buffer for the secret to make it easier to follow it's
lifecycle. For base64 decoding use a local temporary buffer which will
be cleared right away.
This also uses virSecureErase for clearing the bufer instead of
VIR_DISPOSE_N which is being phased out.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Krempa [Mon, 1 Feb 2021 11:12:42 +0000 (12:12 +0100)]
libxlMakeDomBuildInfo: Don't use VIR_DISPOSE_N for USB device list
The list isn't secret which would need being disposed of. Just expand
the array and return failure when adding the NULL terminator similarly
to how we expand the list for adding devices in a loop.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Krempa [Mon, 1 Feb 2021 11:08:13 +0000 (12:08 +0100)]
hypervFreeInvokeParams: Don't use VIR_DISPOSE_N for freeing 'params'
The struct doesn't contain any secrets to clear before freeing and even
if it did VIR_DISPOSE_N wouldn't help as the struct contains only
pointers thus the actual memory pointing to isn't sanitized.
Just free the params array pointer and then the struct itself.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tim Wiederhake [Mon, 1 Feb 2021 12:42:06 +0000 (13:42 +0100)]
vircryptotest: Directly assign string to avoid memcpy
Found by clang-tidy's "bugprone-not-null-terminated-result" check.
clang-tidy's finding is a false positive in this case, as the
memset call guarantees null termination. The assignment can be
simplified though, and this happens to silence the warning.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Tim Wiederhake [Mon, 1 Feb 2021 12:42:02 +0000 (13:42 +0100)]
Replace bzero() with memset()
This was found by clang-tidy's
"clang-analyzer-security.insecureAPI.bzero" check.
bzero is marked as deprecated ("LEGACY") in POSIX.1-2001 and
removed in POSIX.1-2008.
Besides its deprecation, bzero can be unsafe to use under certain
circumstances, e.g. when used to zero-out memory containing secrects.
These calls can be optimized away by the compiler, if it concludes no
further access happens to the memory, thus leaving the secrets still
in memory. Hence its classification as "insecureAPI".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Matt Coleman [Tue, 2 Feb 2021 00:48:36 +0000 (19:48 -0500)]
hyperv: XML parsing of serial ports
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal Privoznik [Wed, 25 Nov 2020 09:40:50 +0000 (10:40 +0100)]
virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
What code tries to achieve is that if no flags were provided to
either 'setmem' or 'setmaxmem' commands then the old (no flags)
API is called to be able to communicate with older daemons.
Well, the code can be simplified a bit.
Note that with this change the old no flag version of APIs is
used more often. Previously if --current argument was given it
resulted in *Flags() version to be called even though it is not
necessary - VIR_DOMAIN_AFFECT_CURRENT is implied.
Therefore, this change in fact allows virsh to talk with broader
set of daemons. No other user visible changes were made.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Laine Stump [Sat, 30 Jan 2021 19:05:50 +0000 (14:05 -0500)]
conf: replace VIR_FREE() with g_free() in vir*Free() functions
This patch takes on one set of examples of unnecessary use of
VIR_FREE() when g_free() is adequate - it modifies only vir*Free()
functions within the conf directory that take a single pointer and
free the object pointed to by that argument before returning. The
modification is to replace VIR_FREE() with g_free() for the object
itself *and* for all subordinate chunks of memory pointed to by that
object.
(NB: there are other functions that VIR_FREE subordinate memory of
objects that end up being freed before return (also sometimes with
VIR_FREE); I am purposefully ignoring those to reduce scope and focus
on a sub class where the pointlessness is obvious.)
Signed-off-by: Laine Stump <laine@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Laine Stump [Fri, 29 Jan 2021 05:12:28 +0000 (00:12 -0500)]
util: rename virStorageEncryptionInfoDefFree()
usually a function call vir*Free() will take a single pointer to an
object as its argument, and will then free all resources associated
with that object, including the object
itself. virStorageEnctyptionInfoDefFree() doesn't do that - it frees
all the subordinate resources of the ojbect, but doesn't free the
object itself; usually a function like that is called
vir*Clear(). Let's rename this function to not be misleading.
Signed-off-by: Laine Stump <laine@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Laine Stump [Fri, 29 Jan 2021 04:39:33 +0000 (23:39 -0500)]
conf: eliminate pointless setting of interface model
There is no point in setting the interface model to unknown during
virDomainNetDefFree(), since we are about to free the object anyway
(and the model isn't used anywhere in the rest of the function).
Signed-off-by: Laine Stump <laine@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
projects / thirdparty / libvirt.git / log
