[libvirt] [PATCH 0/4] virchrdev: Couple of cleanups
Patches 1/4 and 2/4 fix some memleaks, the rest cleans up the code a bit. Michal Prívozník (4): virchrdev: Don't leak @dev member of virChrdevHashEntry struct virchrdev: Don't leak mutex if virChrdevAlloc() fails virchrdev: Use more g_autofree and VIR_AUTOCLOSE virchrdev: Drop needless 'cleanup' label in virChrdevLockFileCreate() src/conf/virchrdev.c | 48 ++++++++++++++++---------------------------- 1 file changed, 17 insertions(+), 31 deletions(-) -- 2.24.1
When opening a console to a domain, we put a tuple of {path, virStreamPtr} into a hash table that's private to the domain. This is to ensure only one client at most has the console stream open. Later, when the console is closed, the tuple is removed from the hash table and freed. Except, @path won't be freed. ==234102== 60 bytes in 5 blocks are definitely lost in loss record 436 of 651 ==234102== at 0x4836753: malloc (vg_replace_malloc.c:307) ==234102== by 0x5549110: g_malloc (in /usr/lib64/libglib-2.0.so.0.6000.6) ==234102== by 0x5562D1E: g_strdup (in /usr/lib64/libglib-2.0.so.0.6000.6) ==234102== by 0x4A5A917: virChrdevOpen (virchrdev.c:412) ==234102== by 0x17B64645: qemuDomainOpenConsole (qemu_driver.c:17309) ==234102== by 0x4BC8031: virDomainOpenConsole (libvirt-domain.c:9662) ==234102== by 0x13F854: remoteDispatchDomainOpenConsole (remote_daemon_dispatch_stubs.h:9211) ==234102== by 0x13F72F: remoteDispatchDomainOpenConsoleHelper (remote_daemon_dispatch_stubs.h:9178) ==234102== by 0x4AB0685: virNetServerProgramDispatchCall (virnetserverprogram.c:430) ==234102== by 0x4AB01F0: virNetServerProgramDispatch (virnetserverprogram.c:302) ==234102== by 0x4AB700B: virNetServerProcessMsg (virnetserver.c:136) ==234102== by 0x4AB70CB: virNetServerHandleJob (virnetserver.c:153) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virchrdev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index d4ca3188c5..7657c41ece 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -225,6 +225,7 @@ static void virChrdevHashEntryFree(void *data) /* delete lock file */ virChrdevLockFileRemove(ent->dev); + g_free(ent->dev); g_free(ent); } -- 2.24.1
On Wed, Jan 08, 2020 at 09:18:29AM +0100, Michal Privoznik wrote:
When opening a console to a domain, we put a tuple of {path, virStreamPtr} into a hash table that's private to the domain. This is to ensure only one client at most has the console stream open. Later, when the console is closed, the tuple is removed from the hash table and freed. Except, @path won't be freed.
==234102== 60 bytes in 5 blocks are definitely lost in loss record 436 of 651 ==234102== at 0x4836753: malloc (vg_replace_malloc.c:307) ==234102== by 0x5549110: g_malloc (in /usr/lib64/libglib-2.0.so.0.6000.6) ==234102== by 0x5562D1E: g_strdup (in /usr/lib64/libglib-2.0.so.0.6000.6) ==234102== by 0x4A5A917: virChrdevOpen (virchrdev.c:412) ==234102== by 0x17B64645: qemuDomainOpenConsole (qemu_driver.c:17309) ==234102== by 0x4BC8031: virDomainOpenConsole (libvirt-domain.c:9662) ==234102== by 0x13F854: remoteDispatchDomainOpenConsole (remote_daemon_dispatch_stubs.h:9211) ==234102== by 0x13F72F: remoteDispatchDomainOpenConsoleHelper (remote_daemon_dispatch_stubs.h:9178) ==234102== by 0x4AB0685: virNetServerProgramDispatchCall (virnetserverprogram.c:430) ==234102== by 0x4AB01F0: virNetServerProgramDispatch (virnetserverprogram.c:302) ==234102== by 0x4AB700B: virNetServerProcessMsg (virnetserver.c:136) ==234102== by 0x4AB70CB: virNetServerHandleJob (virnetserver.c:153)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
This is only a theoretical leak, but in virChrdevAlloc() we initialize a mutex and if creating a hash table fails, then virChrdevFree() is called which because of incorrect check doesn't deinit the mutex. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virchrdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index 7657c41ece..6e659a3783 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -311,7 +311,7 @@ static int virChrdevFreeClearCallbacks(void *payload, */ void virChrdevFree(virChrdevsPtr devs) { - if (!devs || !devs->hash) + if (!devs) return; virMutexLock(&devs->lock); -- 2.24.1
On Wed, Jan 08, 2020 at 09:18:30AM +0100, Michal Privoznik wrote:
This is only a theoretical leak, but in virChrdevAlloc() we initialize a mutex and if creating a hash table fails, then virChrdevFree() is called which because of incorrect check doesn't deinit the mutex.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virchrdev.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index 6e659a3783..766c264472 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -67,9 +67,9 @@ struct _virChrdevStreamInfo { */ static char *virChrdevLockFilePath(const char *dev) { - char *path = NULL; - char *sanitizedPath = NULL; - char *devCopy; + g_autofree char *path = NULL; + g_autofree char *sanitizedPath = NULL; + g_autofree char *devCopy = NULL; char *filename; char *p; @@ -92,10 +92,7 @@ static char *virChrdevLockFilePath(const char *dev) sanitizedPath = virFileSanitizePath(path); - VIR_FREE(path); - VIR_FREE(devCopy); - - return sanitizedPath; + return g_steal_pointer(&sanitizedPath); } /** @@ -107,10 +104,10 @@ static char *virChrdevLockFilePath(const char *dev) */ static int virChrdevLockFileCreate(const char *dev) { - char *path = NULL; + g_autofree char *path = NULL; int ret = -1; - int lockfd = -1; - char *pidStr = NULL; + g_autofree char *pidStr = NULL; + VIR_AUTOCLOSE lockfd = -1; pid_t pid; /* build lock file path */ @@ -161,7 +158,6 @@ static int virChrdevLockFileCreate(const char *dev) _("Couldn't write to lock file for " "device '%s' in path '%s'"), dev, path); - VIR_FORCE_CLOSE(lockfd); unlink(path); goto cleanup; } @@ -170,9 +166,6 @@ static int virChrdevLockFileCreate(const char *dev) ret = 0; cleanup: - VIR_FORCE_CLOSE(lockfd); - VIR_FREE(path); - VIR_FREE(pidStr); return ret; } @@ -184,10 +177,8 @@ static int virChrdevLockFileCreate(const char *dev) */ static void virChrdevLockFileRemove(const char *dev) { - char *path = virChrdevLockFilePath(dev); - if (path) - unlink(path); - VIR_FREE(path); + g_autofree char *path = virChrdevLockFilePath(dev); + unlink(path); } #else /* #ifdef VIR_CHRDEV_LOCK_FILE_PATH */ /* file locking for character devices is disabled */ -- 2.24.1
...
@@ -184,10 +177,8 @@ static int virChrdevLockFileCreate(const char *dev) */ static void virChrdevLockFileRemove(const char *dev) { - char *path = virChrdevLockFilePath(dev); - if (path) - unlink(path); - VIR_FREE(path); + g_autofree char *path = virChrdevLockFilePath(dev); + unlink(path);
I assume that you deleted the 'if' clause only because virChrdevLockFileRemove is only called from virChrdevHashEntryFree and the existence of the lockfile is therefore implicitly assumed? Because I'm not sure unlink can handle NULL, especially if it blindly calls strlen on the input. With your assurance of the above: Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 1/16/20 2:02 PM, Erik Skultety wrote:
...
@@ -184,10 +177,8 @@ static int virChrdevLockFileCreate(const char *dev) */ static void virChrdevLockFileRemove(const char *dev) { - char *path = virChrdevLockFilePath(dev); - if (path) - unlink(path); - VIR_FREE(path); + g_autofree char *path = virChrdevLockFilePath(dev); + unlink(path);
I assume that you deleted the 'if' clause only because virChrdevLockFileRemove is only called from virChrdevHashEntryFree and the existence of the lockfile is therefore implicitly assumed? Because I'm not sure unlink can handle NULL, especially if it blindly calls strlen on the input.
The virChrdevLockFilePath() can return NULL on OOM. However, we would crash on OOM, so we won't even get to unlink()
With your assurance of the above: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Thanks, Michal
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virchrdev.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index 766c264472..800e82869e 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -105,14 +105,13 @@ static char *virChrdevLockFilePath(const char *dev) static int virChrdevLockFileCreate(const char *dev) { g_autofree char *path = NULL; - int ret = -1; g_autofree char *pidStr = NULL; VIR_AUTOCLOSE lockfd = -1; pid_t pid; /* build lock file path */ if (!(path = virChrdevLockFilePath(dev))) - goto cleanup; + return -1; /* check if a log file and process holding the lock still exists */ if (virPidFileReadPathIfAlive(path, &pid, NULL) == 0 && pid >= 0) { @@ -121,7 +120,7 @@ static int virChrdevLockFileCreate(const char *dev) _("Requested device '%s' is locked by " "lock file '%s' held by process %lld"), dev, path, (long long) pid); - goto cleanup; + return -1; } else { /* clean up the stale/corrupted/nonexistent lockfile */ unlink(path); @@ -142,14 +141,13 @@ static int virChrdevLockFileCreate(const char *dev) if (errno == EACCES && geteuid() != 0) { VIR_DEBUG("Skipping lock file creation for device '%s in path '%s'.", dev, path); - ret = 0; - goto cleanup; + return 0; } virReportSystemError(errno, _("Couldn't create lock file for " "device '%s' in path '%s'"), dev, path); - goto cleanup; + return -1; } /* write the pid to the file */ @@ -159,15 +157,11 @@ static int virChrdevLockFileCreate(const char *dev) "device '%s' in path '%s'"), dev, path); unlink(path); - goto cleanup; + return -1; } /* we hold the lock */ - ret = 0; - - cleanup: - - return ret; + return 0; } /** -- 2.24.1
On Wed, Jan 08, 2020 at 09:18:32AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety -
Michal Privoznik