[PATCH 2/2] virmacmap: Don't use hash table dataFree callback

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list