Matthias Bolte wrote: > 2009/11/25 Jim Fehlig <jfehlig@xxxxxxxxxx>: > >> Commit 790f0b3057787bb64da8c46c111ff8d3eff7b2af causes contents of names >> array to be freed even on success, resulting in no listing of defined >> but inactive Xen domains. Patch below fixes it. >> >> Regards, >> Jim >> >> > > Good catch, I just reviewed this commit to see if I've caused similar > bugs elsewhere, but this seems to be the only one. > I had looked elsewhere in that commit as well but didn't see any other problems. > >> Index: libvirt-0.7.4/src/xen/xend_internal.c >> =================================================================== >> --- libvirt-0.7.4.orig/src/xen/xend_internal.c >> +++ libvirt-0.7.4/src/xen/xend_internal.c >> @@ -4693,13 +4693,14 @@ xenDaemonListDefinedDomains(virConnectPt >> } >> >> if (ret >= maxnames) >> - break; >> + goto out; >> } >> >> error: >> for (i = 0; i < ret; ++i) >> VIR_FREE(names[i]); >> >> +out: >> sexpr_free(root); >> return(ret); >> } >> > > Your patch doesn't fix the problem in all situations. If maxnames is > larger than the actual number of domains then goto out is never > executed. > Ah, yes - good catch. > I also forgot to set ret to -1 after freeing the names, this needs to > be fixed too. > > I propose the attached patch to solve this issues. > > Matthias > > diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c > index d61e9e6..a0c7d77 100644 > --- a/src/xen/xend_internal.c > +++ b/src/xen/xend_internal.c > @@ -4696,12 +4696,17 @@ xenDaemonListDefinedDomains(virConnectPtr conn, char **const names, int maxnames > break; > } > > +cleanup: > + sexpr_free(root); > + return(ret); > + > error: > for (i = 0; i < ret; ++i) > VIR_FREE(names[i]); > > - sexpr_free(root); > - return(ret); > + ret = -1; > + > + goto cleanup; > } > > /** > ACK. Thanks again! Jim -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list