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

[Hiroyuki Kaguchi]

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;
             }
         }
     }