2:18 a.m.
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
2:18 a.m.
New subject: [libvirt] [PATCH 1/4] virchrdev: Don't leak @dev member of virChrdevHashEntry struct
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(a)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
6:32 a.m.
New subject: [libvirt] [PATCH 1/4] virchrdev: Don't leak @dev member of virChrdevHashEntry struct
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(a)redhat.com>
Reviewed-by: Erik
Skultety <eskultet(a)redhat.com>
2:18 a.m.
New subject: [libvirt] [PATCH 2/4] virchrdev: Don't leak mutex if virChrdevAlloc() fails
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(a)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
7:03 a.m.
New subject: [libvirt] [PATCH 2/4] virchrdev: Don't leak mutex if virChrdevAlloc() fails
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(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
2:18 a.m.
New subject: [libvirt] [PATCH 3/4] virchrdev: Use more g_autofree and VIR_AUTOCLOSE
Signed-off-by: Michal Privoznik <mprivozn(a)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
7:02 a.m.
New subject: [libvirt] [PATCH 3/4] virchrdev: Use more g_autofree and VIR_AUTOCLOSE
...
@@ -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(a)redhat.com>
7:15 a.m.
New subject: [libvirt] [PATCH 3/4] virchrdev: Use more g_autofree and VIR_AUTOCLOSE
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(a)redhat.com>
Thanks,
Michal
2:18 a.m.
New subject: [libvirt] [PATCH 4/4] virchrdev: Drop needless 'cleanup' label in virChrdevLockFileCreate()
Signed-off-by: Michal Privoznik <mprivozn(a)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
7:02 a.m.
New subject: [libvirt] [PATCH 4/4] virchrdev: Drop needless 'cleanup' label in virChrdevLockFileCreate()
On Wed, Jan 08, 2020 at 09:18:32AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
1772
days inactive
1780
days old
9 comments
2 participants
participants (2)
-
Erik Skultety -
Michal Privoznik