[libvirt] [PATCH 0/2] virmacmap: Fix crash
I was notified about a virmacmaptest crash that happens on FreeBSD. Interestingly, the problem occurs on Linux too, but for some reason the process is not getting SIGSEGV. Michal Privoznik (2): virmacmap: Fix variable handling virmacmap: Don't use hash table dataFree callback src/util/virmacmap.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) -- 2.11.0
In virMacMapRemoveLocked() we have two variables: @macsList and @newMacsList. Obviously, @newMacsList is supposed to hold pointer to modified list but in fact it holds pointer to the old list. It's confusing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virmacmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index 36c364e10..11b3e0334 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -121,8 +121,8 @@ virMacMapRemoveLocked(virMacMapPtr mgr, return 0; newMacsList = macsList; - virStringListRemove(&macsList, mac); - if (!macsList) { + virStringListRemove(&newMacsList, mac); + if (!newMacsList) { virHashSteal(mgr->macs, domain); } else { if (macsList != newMacsList && -- 2.11.0
Due to nature of operations we do over the string list (more precisely due to how virStringListRemove() works), it is not the best idea to use dataFree callback. Problem is, on MAC address remove, the string list remove function modifies the original list in place. Then, virHashUpdateEntry() is called which frees all the data stored in the list rendering @newMacsList point to freed data. ==16002== Invalid read of size 8 ==16002== at 0x50BC083: virFree (viralloc.c:582) ==16002== by 0x513DC39: virStringListFree (virstring.c:251) ==16002== by 0x51089B4: virMacMapHashFree (virmacmap.c:67) ==16002== by 0x50EF30B: virHashAddOrUpdateEntry (virhash.c:352) ==16002== by 0x50EF4FD: virHashUpdateEntry (virhash.c:415) ==16002== by 0x5108BED: virMacMapRemoveLocked (virmacmap.c:129) ==16002== by 0x51092D5: virMacMapRemove (virmacmap.c:346) ==16002== by 0x402F02: testMACRemove (virmacmaptest.c:107) ==16002== by 0x403F15: virTestRun (testutils.c:180) ==16002== by 0x4032C4: mymain (virmacmaptest.c:205) ==16002== by 0x405A3B: virTestMain (testutils.c:992) ==16002== by 0x403D87: main (virmacmaptest.c:237) ==16002== Address 0xdd5a4d0 is 0 bytes inside a block of size 24 free'd ==16002== at 0x4C2AD6F: realloc (vg_replace_malloc.c:693) ==16002== by 0x50BB99B: virReallocN (viralloc.c:245) ==16002== by 0x513DC0B: virStringListRemove (virstring.c:235) ==16002== by 0x5108BA6: virMacMapRemoveLocked (virmacmap.c:124) ==16002== by 0x51092D5: virMacMapRemove (virmacmap.c:346) ==16002== by 0x402F02: testMACRemove (virmacmaptest.c:107) ==16002== by 0x403F15: virTestRun (testutils.c:180) ==16002== by 0x4032C4: mymain (virmacmaptest.c:205) ==16002== by 0x405A3B: virTestMain (testutils.c:992) ==16002== by 0x403D87: main (virmacmaptest.c:237) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virmacmap.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index 11b3e0334..a9697e3d9 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -53,21 +53,25 @@ struct virMacMap { static virClassPtr virMacMapClass; +static int +virMacMapHashFree(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virStringListFree(payload); + return 0; +} + + static void virMacMapDispose(void *obj) { virMacMapPtr mgr = obj; + virHashForEach(mgr->macs, virMacMapHashFree, NULL); virHashFree(mgr->macs); } -static void -virMacMapHashFree(void *payload, const void *name ATTRIBUTE_UNUSED) -{ - virStringListFree(payload); -} - - static int virMacMapOnceInit(void) { if (!(virMacMapClass = virClassNew(virClassForObjectLockable(), @@ -88,19 +92,20 @@ virMacMapAddLocked(virMacMapPtr mgr, const char *mac) { int ret = -1; - const char **macsList = NULL; + char **macsList = NULL; char **newMacsList = NULL; if ((macsList = virHashLookup(mgr->macs, domain)) && - virStringListHasString(macsList, mac)) { + virStringListHasString((const char**) macsList, mac)) { ret = 0; goto cleanup; } - if (!(newMacsList = virStringListAdd(macsList, mac)) || + if (!(newMacsList = virStringListAdd((const char **) macsList, mac)) || virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0) goto cleanup; newMacsList = NULL; + virStringListFree(macsList); ret = 0; cleanup: @@ -303,8 +308,7 @@ virMacMapNew(const char *file) return NULL; virObjectLock(mgr); - if (!(mgr->macs = virHashCreate(VIR_MAC_HASH_TABLE_SIZE, - virMacMapHashFree))) + if (!(mgr->macs = virHashCreate(VIR_MAC_HASH_TABLE_SIZE, NULL))) goto error; if (file && -- 2.11.0
Michal Privoznik wrote:
I was notified about a virmacmaptest crash that happens on FreeBSD. Interestingly, the problem occurs on Linux too, but for some reason the process is not getting SIGSEGV.
Michal Privoznik (2): virmacmap: Fix variable handling virmacmap: Don't use hash table dataFree callback
src/util/virmacmap.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
ACK This looks reasonable and fixes the crash for me. Roman Bogorodskiy
participants (2)
-
Michal Privoznik -
Roman Bogorodskiy