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

[Laine Stump]

On 9/4/20 4:24 AM, Michal Privoznik wrote:
> When building and populating domain NS a couple of functions is
> called


s/is/are/


> 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);
> -        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;
> +            }
>               /* Some other (critical) error. */
>               return -1;
>           }
>   
> +        isLink = S_ISLNK(item.sb.st_mode);
> +
>           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;
>           }
>   
> -        next = item.target;
> +        g_free(next);


I recall once being reprimanded for calling g_free on a g_autofree() 
pointer (by someone who said that *they* had been reprimanded for same). 
I will say


Reviewed-by: Laine Stump <laine at redhat.com>


but don't want to later be fingered as "the person" who allowed this 
into the code, so I guess maybe look for a way around that (or a signoff 
from whoever it was that made the initial "it's bad to manually free a 
g_autofree pointer" statement). (I don't think it's particularly ugly 
BTW. Sure, it could probably be done in a cleaner manner somehow, but 
sometimes things are just messy; you can move the mess, but you can't 
eliminate it)


> +        next = g_strdup(item.target);
> +
> +        if (!added)
> +            qemuNamespaceMknodItemClear(&item);
>       }
>   
>       return 0;