2011/5/12 Eric Blake <eblake@xxxxxxxxxx>:
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list