2011/5/12 Eric Blake <eblake@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