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@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);
- 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@xxxxxxxxxx>
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;
