2011/5/12 Eric Blake <ebl...@redhat.com>: > This has been present since the introduction of phypAttachDevice > in commit 444fd07a. > > * src/phyp/phyp_driver.c (phypAttachDevice): Don't dereference > NULL. > --- > > Found by clang, but the NULL dereference is very blatant. > > However, I'm worried that this patch, while solving the NULL deref, is > dead wrong. In researching this patch, I also found a memory leak: > phypDomainCreateAndStart mallocs a virDomainDefPtr and passes it > phypBuildLpar, but phypBuildLpar neither stashes that memory into a > hash table nor frees it. If it were to stash it into a table, then > subsequent operations on the same domain should reuse that existing > def, rather than malloc'ing one from scratch. Conversely, if the > driver can reconstruct a def in entirety by reading lpar state, then > def should be freed, and there should be a function to recreate a def > at will. Mallocing a temporary def in functions like phypAttachDevice > is probably the wrong thing to do, when really we want to get at the > domain definition corresponding to the domain we are modifying.
Your patch doesn't make it worse as it already is and it avoids a segfault. Yes, the virDomainDefPtr handling needs improvement. > src/phyp/phyp_driver.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c > index 71a3d29..3b4235c 100644 > --- a/src/phyp/phyp_driver.c > +++ b/src/phyp/phyp_driver.c > @@ -1716,14 +1716,19 @@ phypAttachDevice(virDomainPtr domain, const char *xml) > char *vios_name = NULL; > char *profile = NULL; > virDomainDeviceDefPtr dev = NULL; > virDomainDefPtr def = NULL; > virBuffer buf = VIR_BUFFER_INITIALIZER; > char *domain_name = NULL; > > + if (VIR_ALLOC(def) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + > domain_name = escape_specialcharacters(domain->name); > > if (domain_name == NULL) { > goto cleanup; > } > > def->os.type = strdup("aix"); > -- > 1.7.4.4 ACK, to the fix of the directly problem here. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list