Re: [Qemu-block] [Qemu-devel] [PATCH for-2.6] crypto: Avoid memory leak on failure

[Eric Blake]

On 04/07/2016 09:14 AM, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 01.04.2016 17:57, Eric Blake wrote:
>>> Commit 7836857 introduced a memory leak due to invalid use of
>>> Error vs. visit_type_end().  If visiting the intermediate
>>> members fails, we clear the error and unconditionally use
>>> visit_end_struct() on the same error object; but if that
>>> cleanup succeeds, we then skip the qapi_free call.
>>
>> It's not really a memleak. Due to skipping those conditional branches
>> after the "out" label, a non-null value will be returned. In order to
>> determine whether the function call failed, the callers of these
>> functions do not use the errp value but the return value. Therefore,
>> they will think the call succeeded when actually it did not.
> 
> Please amend the commit message accordingly.

Too late; already merged as 95c3df5a.  [And welcome back - hope you
don't mind the backlog...]

(Locally it looks like a memory leak; it is only the wider analysis that
shows that the caller is not leaking things, but where the bug then
shifts to being a potential for the caller to abort if it tries to set
an error into the already-set errp)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.6] crypto: Avoid memory leak on failure

[Markus Armbruster]

Max Reitz  writes:

> On 01.04.2016 17:57, Eric Blake wrote:
>> Commit 7836857 introduced a memory leak due to invalid use of
>> Error vs. visit_type_end().  If visiting the intermediate
>> members fails, we clear the error and unconditionally use
>> visit_end_struct() on the same error object; but if that
>> cleanup succeeds, we then skip the qapi_free call.
>
> It's not really a memleak. Due to skipping those conditional branches
> after the "out" label, a non-null value will be returned. In order to
> determine whether the function call failed, the callers of these
> functions do not use the errp value but the return value. Therefore,
> they will think the call succeeded when actually it did not.

Please amend the commit message accordingly.

>> 
>> Until a later patch adds visit_check_struct(), the only safe
>> approach is to use two separate error objects.
>> 
>> Signed-off-by: Eric Blake 
>> ---
>>  block/crypto.c | 12 ++--
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> Anyway, thanks, applied to my block branch:
>
> https://github.com/XanClic/qemu/commits/block
>
> Max