[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]