On Tue, Jan 29, 2008 at 11:32:04AM +0900, Hiroyuki Kaguchi wrote:
> There are two logic error and a unnecessary else-statement
> in virHashRemoveSet function.
Hum, I will have to double check, I cleaned up something in libxml2
recently which may have been similar.
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list