10:52 a.m.
*** BLURB HERE ***
Michal Prívozník (2):
qemu_namespace: Don't leak memory in qemuDomainGetPreservedMounts()
qemu_namespace: Make qemuDomainGetPreservedMounts() more robust wrt
running VMs
src/qemu/qemu_namespace.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
--
2.37.4
10:52 a.m.
New subject: [PATCH 1/2] qemu_namespace: Don't leak memory in qemuDomainGetPreservedMounts()
The aim of qemuDomainGetPreservedMounts() is to get a list of
filesystems mounted under /dev and optionally generate a path for
each one where they are moved temporarily when building the
namespace. And the function tries to be a bit clever about it.
For instance, if /dev/shm mount point exists, there's no need to
consider /dev/shm/a nor /dev/shm/b as preserving just 'top level'
/dev/shm gives the same result. To achieve this, the function
iterates over the list of filesystem as returned by
virFileGetMountSubtree() and removes the nested ones. However, it
does so in a bit clumsy way: plain VIR_DELETE_ELEMENT() is used
without freeing the string itself. Therefore, if all three
aforementioned example paths appeared on the list, /dev/shm/a and
/dev/shm/b strings would be leaked.
And when I think about it more, there's no real need to shrink
the array down (realloc()). It's going to be free()-d when
returning from the function. Switch to
VIR_DELETE_ELEMENT_INPLACE() then.
Fixes: cdd9205dfffa3aaed935446a41f0d2dd1357c268
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_namespace.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 311c66d46e..9fed6871e8 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -159,7 +159,8 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfig *cfg,
if (c && (*c == '/' || *c == '\0')) {
VIR_DEBUG("Dropping path %s because of %s", mounts[j],
mounts[i]);
- VIR_DELETE_ELEMENT(mounts, j, nmounts);
+ VIR_FREE(mounts[j]);
+ VIR_DELETE_ELEMENT_INPLACE(mounts, j, nmounts);
} else {
j++;
}
--
2.37.4
5:41 a.m.
New subject: [PATCH 1/2] qemu_namespace: Don't leak memory in qemuDomainGetPreservedMounts()
On Mon, Oct 31, 2022 at 16:52:59 +0100, Michal Privoznik wrote:
The aim of qemuDomainGetPreservedMounts() is to get a list of
filesystems mounted under /dev and optionally generate a path for
each one where they are moved temporarily when building the
namespace. And the function tries to be a bit clever about it.
For instance, if /dev/shm mount point exists, there's no need to
consider /dev/shm/a nor /dev/shm/b as preserving just 'top level'
/dev/shm gives the same result. To achieve this, the function
iterates over the list of filesystem as returned by
virFileGetMountSubtree() and removes the nested ones. However, it
does so in a bit clumsy way: plain VIR_DELETE_ELEMENT() is used
without freeing the string itself. Therefore, if all three
aforementioned example paths appeared on the list, /dev/shm/a and
/dev/shm/b strings would be leaked.
And when I think about it more, there's no real need to shrink
the array down (realloc()). It's going to be free()-d when
returning from the function. Switch to
VIR_DELETE_ELEMENT_INPLACE() then.
Fixes: cdd9205dfffa3aaed935446a41f0d2dd1357c268
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_namespace.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
5:44 a.m.
New subject: [PATCH 1/2] qemu_namespace: Don't leak memory in qemuDomainGetPreservedMounts()
On a Monday in 2022, Michal Privoznik wrote:
The aim of qemuDomainGetPreservedMounts() is to get a list of
filesystems mounted under /dev and optionally generate a path for
each one where they are moved temporarily when building the
namespace. And the function tries to be a bit clever about it.
For instance, if /dev/shm mount point exists, there's no need to
consider /dev/shm/a nor /dev/shm/b as preserving just 'top level'
/dev/shm gives the same result. To achieve this, the function
iterates over the list of filesystem as returned by
virFileGetMountSubtree() and removes the nested ones. However, it
does so in a bit clumsy way: plain VIR_DELETE_ELEMENT() is used
without freeing the string itself. Therefore, if all three
aforementioned example paths appeared on the list, /dev/shm/a and
/dev/shm/b strings would be leaked.
And when I think about it more, there's no real need to shrink
the array down (realloc()). It's going to be free()-d when
returning from the function. Switch to
VIR_DELETE_ELEMENT_INPLACE() then.
Fixes: cdd9205dfffa3aaed935446a41f0d2dd1357c268
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_namespace.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:53 a.m.
New subject: [PATCH 2/2] qemu_namespace: Make qemuDomainGetPreservedMounts() more robust wrt running VMs
The aim of qemuDomainGetPreservedMounts() is to get a list of
filesystems mounted under /dev and optionally generate a path for
each one where they are moved temporarily when building the
namespace. And if given domain is also running it looks into its
mount table rather than at the host one. But if it did look at
the domain's private mount table, it find /dev mounted twice: the
first time by udev, the second time the tmpfs mounted by us.
Now, later in the function there's a "sorting" algorithm that
tries to reduce number of mount points needing preservation, by
identifying nested mount points. And if we keep the second
occurrence of /dev on the list, well, after the "sorting" we are
left with nothing but "/dev" because all other mount points are
nested.
Fixes: 46b03819ae8d833b11c2aaccb2c2a0361727f51b
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_namespace.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 9fed6871e8..8189cc37ba 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -154,6 +154,17 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfig *cfg,
for (i = 1; i < nmounts; i++) {
size_t j = i + 1;
+ /* If we looked into mount table of already running VM,
+ * we might have found /dev twice. Remove the other
+ * occurrence as it would jeopardize the rest of the prune
+ * algorithm.
+ */
+ if (STREQ(mounts[i], "/dev")) {
+ VIR_FREE(mounts[i]);
+ VIR_DELETE_ELEMENT_INPLACE(mounts, i, nmounts);
+ continue;
+ }
+
while (j < nmounts) {
char *c = STRSKIP(mounts[j], mounts[i]);
--
2.37.4
5:45 a.m.
New subject: [PATCH 2/2] qemu_namespace: Make qemuDomainGetPreservedMounts() more robust wrt running VMs
On a Monday in 2022, Michal Privoznik wrote:
The aim of qemuDomainGetPreservedMounts() is to get a list of
filesystems mounted under /dev and optionally generate a path for
each one where they are moved temporarily when building the
namespace. And if given domain is also running it looks into its
mount table rather than at the host one. But if it did look at
the domain's private mount table, it find /dev mounted twice: the
first time by udev, the second time the tmpfs mounted by us.
Now, later in the function there's a "sorting" algorithm that
tries to reduce number of mount points needing preservation, by
identifying nested mount points. And if we keep the second
occurrence of /dev on the list, well, after the "sorting" we are
left with nothing but "/dev" because all other mount points are
nested.
Fixes: 46b03819ae8d833b11c2aaccb2c2a0361727f51b
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_namespace.c | 11 +++++++++++
1 file changed, 11 insertions(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
781
days inactive
782
days old
5 comments
3 participants
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa