qemu: Avoid memleak on failure to format blockjobs

[libvirt] [PATCH] qemu: Avoid memleak on failure to format blockjobs

Posted by Eric Blake 5 years, 6 months ago

virXMLFormatElement() frees attrBuf on success, but not necessarily
on failure. Most other callers of this function take the time to
reset attrBuf afterwords, but qemuDomainObjPrivateXMLFOrmatBlockjobs()
was relying on it succeeding, and could thus result in a memory leak.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/qemu/qemu_domain.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dd67be5e2a..ad7a6fe136 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2232,11 +2232,15 @@ qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf,
 {
     virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
     bool bj = qemuDomainHasBlockjob(vm, false);
+    ret = -1;

     virBufferAsprintf(&attrBuf, " active='%s'",
                       virTristateBoolTypeToString(virTristateBoolFromBool(bj)));

-    return virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
+    ret = virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
+ cleanup:
+    virBufferFreeAndReset(&attrBuf);
+    return ret;
 }


-- 
2.17.2

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

Re: [libvirt] [PATCH] qemu: Avoid memleak on failure to format blockjobs

Posted by Eric Blake 5 years, 6 months ago

On 10/18/18 3:08 PM, Eric Blake wrote:
> virXMLFormatElement() frees attrBuf on success, but not necessarily
> on failure. Most other callers of this function take the time to
> reset attrBuf afterwords, but qemuDomainObjPrivateXMLFOrmatBlockjobs()
> was relying on it succeeding, and could thus result in a memory leak.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   src/qemu/qemu_domain.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index dd67be5e2a..ad7a6fe136 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2232,11 +2232,15 @@ qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf,
>   {
>       virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
>       bool bj = qemuDomainHasBlockjob(vm, false);
> +    ret = -1;

I need to quit editing patches in my mailer. As written, this obviously 
doesn't compile; s/ret/int ret/

> 
>       virBufferAsprintf(&attrBuf, " active='%s'",
>                         virTristateBoolTypeToString(virTristateBoolFromBool(bj)));
> 
> -    return virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
> +    ret = virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
> + cleanup:

and this label is unused.

> +    virBufferFreeAndReset(&attrBuf);
> +    return ret;
>   }
> 
> 

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

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

Re: [libvirt] [PATCH] qemu: Avoid memleak on failure to format blockjobs

Posted by Michal Privoznik 5 years, 6 months ago

On 10/18/2018 10:42 PM, Eric Blake wrote:
> On 10/18/18 3:08 PM, Eric Blake wrote:
>> virXMLFormatElement() frees attrBuf on success, but not necessarily
>> on failure. Most other callers of this function take the time to
>> reset attrBuf afterwords, but qemuDomainObjPrivateXMLFOrmatBlockjobs()
>> was relying on it succeeding, and could thus result in a memory leak.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   src/qemu/qemu_domain.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index dd67be5e2a..ad7a6fe136 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -2232,11 +2232,15 @@
>> qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf,
>>   {
>>       virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
>>       bool bj = qemuDomainHasBlockjob(vm, false);
>> +    ret = -1;
> 
> I need to quit editing patches in my mailer. As written, this obviously
> doesn't compile; s/ret/int ret/
> 
>>
>>       virBufferAsprintf(&attrBuf, " active='%s'",
>>                        
>> virTristateBoolTypeToString(virTristateBoolFromBool(bj)));
>>
>> -    return virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
>> +    ret = virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
>> + cleanup:
> 
> and this label is unused.
> 
>> +    virBufferFreeAndReset(&attrBuf);
>> +    return ret;
>>   }
>>
>>
> 

ACK with those cleanups squashed in.

Michal

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