On a Friday in 2020, Michal Privoznik wrote:
When building and populating domain NS a couple of functions is
called that append paths to a string list. This string list is
then inspected, one item at the time by
qemuNamespacePrepareOneItem() which gathers all the info for
given path (stat buffer, possible link target, ACLs, SELinux
label) using qemuNamespaceMknodItemInit(). If the path needs to
be created in the domain's private /dev then it's added onto this
qemuNamespaceMknodData list which is freed later in the process.
But, if the path does not need to be created in the domain's
private /dev, then the memory allocated by
qemuNamespaceMknodItemInit() is not freed anywhere leading to a
leak.
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
Honestly, I think this patch looks ugly. Ideas are welcome.
src/qemu/qemu_namespace.c | 42 +++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 87f4fd8d58..917e140f6a 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1166,22 +1166,29 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr data,
size_t ndevMountsPath)
{
long ttl = sysconf(_SC_SYMLOOP_MAX);
- const char *next = file;
+ g_autofree char *next = g_strdup(file);
size_t i;
while (1) {
qemuNamespaceMknodItem item = { 0 };
+ bool added = false;
+ bool isLink;
int rc;
rc = qemuNamespaceMknodItemInit(&item, cfg, vm, next);
@next gets consumed here. That seems to be the cause of all grief.
- if (rc == -2) {
- /* @file doesn't exist. We can break here. */
- break;
- } else if (rc < 0) {
+ if (rc < 0) {
+ qemuNamespaceMknodItemClear(&item);
+
+ if (rc == -2) {
+ /* @file doesn't exist. We can break here. */
+ break;
Strictly speaking, there is nothing to free yet because -2 is returned pretty early. And ItemInit could be changed to clean up after itself if it returns -1.
+ }
/* Some other (critical) error. */
return -1;
}
+ isLink = S_ISLNK(item.sb.st_mode);
+
If you move the next assignment here, nothing below the APPEND_ELEMENT_COPY needs to access item anymore, so you can switch it to the non-copy version and clear it unconditionally and use g_auto to clear it.
if (STRPREFIX(next, QEMU_DEVPREFIX)) {
for (i = 0; i < ndevMountsPath; i++) {
if (STREQ(devMountsPath[i], "/dev"))
@@ -1190,22 +1197,35 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr data,
break;
}
- if (i == ndevMountsPath &&
- VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0)
- return -1;
+ if (i == ndevMountsPath) {
+ if (VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0) {
+ qemuNamespaceMknodItemClear(&item);
+ return -1;
+ }
+ added = true;
+ }
}
- if (!S_ISLNK(item.sb.st_mode))
+ if (!isLink) {
+ if (!added)
+ qemuNamespaceMknodItemClear(&item);
break;
+ }
if (ttl-- == 0) {
+ if (!added)
+ qemuNamespaceMknodItemClear(&item);
virReportSystemError(ELOOP,
_("Too many levels of symbolic links: %s"),
- next);
+ file);
return -1;
This change makes sense on its own and can be separated.
}
- next = item.target;
+ g_free(next);
Apart from the g_free/g_autofree mixup, this frees previous iteration's item->file, possibly after it has been added to data->items. So we do need to g_strdup it in ItemInit.
+ next = g_strdup(item.target); +
If you add a second array to hold the thrown away items during this function, you could leave the iterator const char*. Not sure whether that is nicer or not. Jano
+ if (!added)
+ qemuNamespaceMknodItemClear(&item);
}
return 0;
--
2.26.2
Attachment:
signature.asc
Description: PGP signature
