Re: [libvirt] [PATCH] phyp: avoid a crash

[Matthias Bolte <matthias bolte googlemail com>]

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