8:32 p.m.
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;
}
}
}