Re: [Libvir] [PATCH] Fix the function that remove the element of the hash table.

There are two logic error and a unnecessary else-statement in virHashRemoveSet function.

This patch fix the following. (1/3) The logic error that use released memory area. (2/3) The logic error that doesn't remove elements. (3/3) Unnecessary else-statement. (1/3) The logic error that use released memory area. This bug causes the crash of virt-manager or virsh. If "virsh undefine" command is executed, "Segmentation Fault" is caused. The result of executing "valgrind virsh" command is: virsh # list --all ==3337== Invalid read of size 4 ==3337== at 0x4016A21: virHashRemoveSet (hash.c:562) ==3337== by 0x403ACF4: xenXMConfigCacheRefresh (xm_internal.c:458) ==3337== by 0x4040D58: xenXMNumOfDefinedDomains (xm_internal.c:2518) ==3337== by 0x40245C3: xenUnifiedNumOfDefinedDomains (xen_unified.c:1004) ==3337== by 0x4013730: virConnectNumOfDefinedDomains (libvirt.c:2446) ==3337== by 0x804AE81: cmdList (virsh.c:577) ==3337== by 0x8052E6E: vshCommandRun (virsh.c:4052) ==3337== by 0x8054E2D: main (virsh.c:5036) ==3337== Address 0x4282AB8 is 0 bytes inside a block of size 16 free'd ==3337== at 0x4004FDA: free (vg_replace_malloc.c:233) ==3337== by 0x40169A0: virHashRemoveSet (hash.c:545) ==3337== by 0x403ACF4: xenXMConfigCacheRefresh (xm_internal.c:458) ==3337== by 0x4040D58: xenXMNumOfDefinedDomains (xm_internal.c:2518) ==3337== by 0x40245C3: xenUnifiedNumOfDefinedDomains (xen_unified.c:1004) ==3337== by 0x4013730: virConnectNumOfDefinedDomains (libvirt.c:2446) ==3337== by 0x804AE81: cmdList (virsh.c:577) ==3337== by 0x8052E6E: vshCommandRun (virsh.c:4052) ==3337== by 0x8054E2D: main (virsh.c:5036) The current removing logic is: 1. search for node that be removed in the hash table. The removing cursor(pointer) point to "element-B". +-hash table---+ | +----------+ | +----------+ +----------+ | |element-A |-|-->|element-B |-->|element-C | | +----------+ | +----------+ +----------+ | |element-D | | | +----------+ | | . | | . | +--------------+ 2. free "element-B". The removing cursor point to invalid address +-hash table---+ | +----------+ | +----------+ | |element-A |-|-->|element-C | | +----------+ | +----------+ | |element-D | | | +----------+ | | . | | . | +--------------+ 3. try to move the removing cursor to next node. At this time, the error occurs. (2/3) The logic error that doesn't remove elements. The current removing logic is: 1. search for node that will be removed in the hash table. The removing cursor point to "element-A". +-hash table---+ | +----------+ | +----------+ +----------+ | |element-A |-|-->|element-B |-->|element-C | | +----------+ | +----------+ +----------+ | |element-D | | | +----------+ | | . | | . | +--------------+ 2. copy "element-B" to "element-A". +-hash table---+ | +----------+ | +----------+ +----------+ | |element-B'|-|-->|element-B |-->|element-C | | +----------+ | +----------+ +----------+ | |element-D | | | +----------+ | | . | | . | +--------------+ 3. remove "element-B" The removing cursor point to "element-D". "element-C" is skipped. +-hash table---+ | +----------+ | +----------+ | |element-B'|-|-->|element-C | | +----------+ | +----------+ | |element-D | | | +----------+ | | . | | . | +--------------+ (3/3) Unnecessary else-statement. There is else-statement that set NULL to the NULL pointer. Thanks, Hiroyuki Kaguchi Index: hash.c =================================================================== RCS file: /data/cvs/libvirt/src/hash.c,v retrieving revision 1.27 diff -u -r1.27 hash.c --- hash.c 21 Jan 2008 16:29:10 -0000 1.27 +++ hash.c 28 Jan 2008 06:48:09 -0000 @@ -543,6 +543,7 @@ if (prev) { prev->next = entry->next; free(entry); + entry = prev; } else { if (entry->next == NULL) { entry->valid = 0; @@ -553,6 +554,7 @@ sizeof(virHashEntry)); free(entry); entry = NULL; + i--; } } table->nbElems--; @@ -560,8 +562,6 @@ prev = entry; if (entry) { entry = entry->next; - } else { - entry = NULL; } } } -- Libvir-list mailing list Libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list