[libvirt PATCH 0/5] Fix error handling in nwfilterStateInitialize
See patch 5 for description. Tim Wiederhake (5): virNWFilterObjListFree: Prevent null pointer derefernce virNWFilterSnoopState: Prevent mutex leak nwfilterDriverRemoveDBusMatches: Prevent unsubscribing from null id virNWFilterDriverState: Destroy mutex safely nwfilterStateInitialize: Simplify and fix error handling src/conf/virnwfilterobj.c | 3 ++ src/conf/virnwfilterobj.h | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 15 +++++++-- src/nwfilter/nwfilter_driver.c | 55 ++++++++++++++----------------- 4 files changed, 41 insertions(+), 33 deletions(-) -- 2.31.1
Allow virNWFilterObjListFree to be called with a NULL argument. This enables a later patch to use virNWFilterObjListFree as a cleanup function safely, as it is a no-op if virNWFilterObj was not yet initialized. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virnwfilterobj.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index e9bb2b1811..f9c1b049d5 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -110,6 +110,9 @@ virNWFilterObjFree(virNWFilterObj *obj) void virNWFilterObjListFree(virNWFilterObjList *nwfilters) { + if (!nwfilters) + return; + g_hash_table_unref(nwfilters->objs); g_hash_table_unref(nwfilters->objsName); g_free(nwfilters); -- 2.31.1
virNWFilterDHCPSnoopShutdown would never destroy the mutexes created in virNWFilterDHCPSnoopInit. Additionally, if in virNWFilterDHCPSnoopInit the call to virMutexInitRecursive succeeds and the call to virMutexInit fails, this would lead to either virNWFilterSnoopState.snoopLock being initialized twice or virNWFilterSnoopState.activeLock destroyed without being initialized first. This enables a later patch to use virNWFilterDHCPSnoopShutdown as a cleanup function safely, as it is a no-op if virNWFilterSnoopState was not yet initialized. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 852840c209..d26e787453 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1860,10 +1860,14 @@ virNWFilterDHCPSnoopInit(void) VIR_DEBUG("Initializing DHCP snooping"); - if (virMutexInitRecursive(&virNWFilterSnoopState.snoopLock) < 0 || - virMutexInit(&virNWFilterSnoopState.activeLock) < 0) + if (virMutexInitRecursive(&virNWFilterSnoopState.snoopLock) < 0) return -1; + if (virMutexInit(&virNWFilterSnoopState.activeLock) < 0) { + virMutexDestroy(&virNWFilterSnoopState.snoopLock); + return -1; + } + virNWFilterSnoopState.ifnameToKey = virHashNew(NULL); virNWFilterSnoopState.active = virHashNew(NULL); virNWFilterSnoopState.snoopReqs = @@ -1938,6 +1942,9 @@ virNWFilterDHCPSnoopEnd(const char *ifname) void virNWFilterDHCPSnoopShutdown(void) { + if (!virNWFilterSnoopState.snoopReqs) + return; + virNWFilterSnoopEndThreads(); virNWFilterSnoopJoinThreads(); @@ -1947,9 +1954,13 @@ virNWFilterDHCPSnoopShutdown(void) g_clear_pointer(&virNWFilterSnoopState.snoopReqs, g_hash_table_unref); } + virMutexDestroy(&virNWFilterSnoopState.snoopLock); + VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.activeLock) { g_clear_pointer(&virNWFilterSnoopState.active, g_hash_table_unref); } + + virMutexDestroy(&virNWFilterSnoopState.activeLock); } #else /* WITH_LIBPCAP */ -- 2.31.1
Allow nwfilterDriverRemoveDBusMatches to be called without nwfilterDriverInstallDBusMatches being called previously. This enables a later patch to use nwfilterDriverRemoveDBusMatches as a cleanup function safely. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_driver.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 1f7d40e1b0..f89b5b8757 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -77,12 +77,19 @@ static unsigned int reloadID; static void nwfilterDriverRemoveDBusMatches(void) { - GDBusConnection *sysbus; + GDBusConnection *sysbus = virGDBusGetSystemBus(); - sysbus = virGDBusGetSystemBus(); - if (sysbus) { + if (!sysbus) + return; + + if (restartID != 0) { g_dbus_connection_signal_unsubscribe(sysbus, restartID); + restartID = 0; + } + + if (reloadID != 0) { g_dbus_connection_signal_unsubscribe(sysbus, reloadID); + reloadID = 0; } } -- 2.31.1
Allow nwfilterStateCleanupLocked to be called on a partially constructed driver object. This enables the next patch to simplify and fix error handling in nwfilterStateInitialize. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virnwfilterobj.h | 1 + src/nwfilter/nwfilter_driver.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index cb103280e8..b67dc017c5 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -45,6 +45,7 @@ struct _virNWFilterDriverState { /* Recursive. Hold for filter changes, instantiation or deletion */ virMutex updateLock; + bool updateLockInitialized; }; virNWFilterDef * diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index f89b5b8757..bf17c5ea66 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -173,7 +173,8 @@ nwfilterStateCleanupLocked(void) /* free inactive nwfilters */ virNWFilterObjListFree(driver->nwfilters); - virMutexDestroy(&driver->updateLock); + if (driver->updateLockInitialized) + virMutexDestroy(&driver->updateLock); g_clear_pointer(&driver, g_free); return 0; @@ -222,6 +223,7 @@ nwfilterStateInitialize(bool privileged, if (virMutexInitRecursive(&driver->updateLock) < 0) goto err_free_driverstate; + driver->updateLockInitialized = true; driver->privileged = privileged; if (!(driver->nwfilters = virNWFilterObjListNew())) -- 2.31.1
Under certain circumstances nwfilterStateInitialize could leak memory: If e.g. the call to virNWFilterConfLayerInit fails, the error path err_techdrivers_shutdown does not free the previously allocated memory held in driver->stateDir. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_driver.c | 38 ++++++++++------------------------ 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index bf17c5ea66..b66ba22737 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -210,18 +210,17 @@ nwfilterStateInitialize(bool privileged, if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Driver does not support embedded mode")); - return -1; + return VIR_DRV_STATE_INIT_ERROR; } - if (virGDBusHasSystemBus() && - !(sysbus = virGDBusGetSystemBus())) + if (virGDBusHasSystemBus() && !(sysbus = virGDBusGetSystemBus())) return VIR_DRV_STATE_INIT_ERROR; driver = g_new0(virNWFilterDriverState, 1); driver->lockFD = -1; if (virMutexInitRecursive(&driver->updateLock) < 0) - goto err_free_driverstate; + goto error; driver->updateLockInitialized = true; driver->privileged = privileged; @@ -248,18 +247,19 @@ nwfilterStateInitialize(bool privileged, goto error; if (virNWFilterIPAddrMapInit() < 0) - goto err_free_driverstate; + goto error; + if (virNWFilterLearnInit() < 0) - goto err_exit_ipaddrmapshutdown; + goto error; + if (virNWFilterDHCPSnoopInit() < 0) - goto err_exit_learnshutdown; + goto error; if (virNWFilterTechDriversInit(privileged) < 0) - goto err_dhcpsnoop_shutdown; + goto error; - if (virNWFilterConfLayerInit(virNWFilterTriggerRebuildImpl, - driver) < 0) - goto err_techdrivers_shutdown; + if (virNWFilterConfLayerInit(virNWFilterTriggerRebuildImpl, driver) < 0) + goto error; /* * startup the DBus late so we don't get a reload signal while @@ -297,22 +297,6 @@ nwfilterStateInitialize(bool privileged, error: nwfilterStateCleanupLocked(); - - return VIR_DRV_STATE_INIT_ERROR; - - err_techdrivers_shutdown: - virNWFilterTechDriversShutdown(); - err_dhcpsnoop_shutdown: - virNWFilterDHCPSnoopShutdown(); - err_exit_learnshutdown: - virNWFilterLearnShutdown(); - err_exit_ipaddrmapshutdown: - virNWFilterIPAddrMapShutdown(); - - err_free_driverstate: - virNWFilterObjListFree(driver->nwfilters); - g_clear_pointer(&driver, g_free); - return VIR_DRV_STATE_INIT_ERROR; } -- 2.31.1
On 4/8/22 15:12, Tim Wiederhake wrote:
See patch 5 for description.
Tim Wiederhake (5): virNWFilterObjListFree: Prevent null pointer derefernce virNWFilterSnoopState: Prevent mutex leak nwfilterDriverRemoveDBusMatches: Prevent unsubscribing from null id virNWFilterDriverState: Destroy mutex safely nwfilterStateInitialize: Simplify and fix error handling
src/conf/virnwfilterobj.c | 3 ++ src/conf/virnwfilterobj.h | 1 + src/nwfilter/nwfilter_dhcpsnoop.c | 15 +++++++-- src/nwfilter/nwfilter_driver.c | 55 ++++++++++++++----------------- 4 files changed, 41 insertions(+), 33 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake