Re: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser

[Marc Hartmayer]

On Fri, Apr 06, 2018 at 06:53 PM +0200, John Ferlan  wrote:
> Rather than using VIR_ALLOC, use the New API since we already
> use the virDomainChrSourceDefFree function when done.
>
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index aacd06a87a..caf3f47c63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11138,7 +11138,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  goto error;
>  }
>
> -if (VIR_ALLOC(def->data.vhostuser) < 0)
> +if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
>  goto error;
>
>  def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>

Reviewed-by: Marc Hartmayer 

--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser

[Eric Blake]

On 04/06/2018 01:31 PM, Laine Stump wrote:
> On 04/06/2018 12:53 PM, John Ferlan wrote:
>> Rather than using VIR_ALLOC, use the New API since we already
>> use the virDomainChrSourceDefFree function when done.
>>

>> -if (VIR_ALLOC(def->data.vhostuser) < 0)
>> +if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
>>  goto error;
>>  
>>  def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> 
> 
> Reviewed-by: Laine Stump 
> 
> 
> There have been cases in the past where a *New() or *Free() function was
> added but not consistently used, leading to leaks or (worse, but at
> least easier to detect) crashes due to null pointer dereferences. It
> would really be nice if there was some way to automate the auditing of
> all the VIR_ALLOCs, but I don't think it's possible with the simple make
> syntax-check target (that's supposed to be a challenge to someone :-),
> and there's 1223 uses of VIR_ALLOC() (6421 of VIR_FREE()), so even a one
> time manual audit is really out of the question (and the results would
> be immediately obsolete as soon as a new VIR_ALLOC or VIR_FREE was added).

I wonder - is there some way we could automate the auditing, perhaps in
a debug-only mode, by expanding the VIR_ALLOC() macro to do a giant
compile-time blacklist using a chain of gcc's __builtin_choose_expr()
and __builtin_types_compatible_p(ptr, BadType) to forbid direct use of
VIR_ALLOC() on any type we add to the blacklist chain.  Meanwhile, we'd
have to add a new variant of VIR_ALLOC() for use within call sites like
virDomainChrSourceDevNew(), that are performing the actual allocation
for any type that is otherwise blacklisted (and where it is easier to
audit that calls to the new macro are really limited to the constructor
functions of those blacklisted types).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser

[Laine Stump]

On 04/06/2018 12:53 PM, John Ferlan wrote:
> Rather than using VIR_ALLOC, use the New API since we already
> use the virDomainChrSourceDefFree function when done.
>
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index aacd06a87a..caf3f47c63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11138,7 +11138,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  goto error;
>  }
>  
> -if (VIR_ALLOC(def->data.vhostuser) < 0)
> +if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
>  goto error;
>  
>  def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;


Reviewed-by: Laine Stump 


There have been cases in the past where a *New() or *Free() function was
added but not consistently used, leading to leaks or (worse, but at
least easier to detect) crashes due to null pointer dereferences. It
would really be nice if there was some way to automate the auditing of
all the VIR_ALLOCs, but I don't think it's possible with the simple make
syntax-check target (that's supposed to be a challenge to someone :-),
and there's 1223 uses of VIR_ALLOC() (6421 of VIR_FREE()), so even a one
time manual audit is really out of the question (and the results would
be immediately obsolete as soon as a new VIR_ALLOC or VIR_FREE was added).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list