[PATCH 3/3] qemu_namespace: Don't leak mknod items that are being skipped over

[Ján Tomko]

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 at redhat.com>
>---
>
>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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200904/b0b6eab7/attachment-0001.sig>