From 5dc6169bc803c05d499471b0686ceb9da8290d07 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Mon, 2 Jan 2017 10:35:33 +0100 Subject: [PATCH] virmacmap: Don't use hash table dataFree callback 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 --- src/util/virmacmap.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index 11b3e03346..a9697e3d93 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -53,18 +53,22 @@ struct virMacMap { static virClassPtr virMacMapClass; -static void -virMacMapDispose(void *obj) +static int +virMacMapHashFree(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { - virMacMapPtr mgr = obj; - virHashFree(mgr->macs); + virStringListFree(payload); + return 0; } static void -virMacMapHashFree(void *payload, const void *name ATTRIBUTE_UNUSED) +virMacMapDispose(void *obj) { - virStringListFree(payload); + virMacMapPtr mgr = obj; + virHashForEach(mgr->macs, virMacMapHashFree, NULL); + virHashFree(mgr->macs); } @@ -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.39.5