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

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

[Eric Blake]

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.

Until a later patch adds visit_check_struct(), the only safe
approach is to use two separate error objects.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/crypto.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index be34985..1903e84 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -196,6 +196,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
     OptsVisitor *ov;
     QCryptoBlockOpenOptions *ret = NULL;
     Error *local_err = NULL;
+    Error *end_err = NULL;

     ret = g_new0(QCryptoBlockOpenOptions, 1);
     ret->format = format;
@@ -218,10 +219,9 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
         error_setg(&local_err, "Unsupported block format %d", format);
         break;
     }
-    error_propagate(errp, local_err);
-    local_err = NULL;

-    visit_end_struct(opts_get_visitor(ov), &local_err);
+    visit_end_struct(opts_get_visitor(ov), &end_err);
+    error_propagate(&local_err, end_err);

  out:
     if (local_err) {
@@ -242,6 +242,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
     OptsVisitor *ov;
     QCryptoBlockCreateOptions *ret = NULL;
     Error *local_err = NULL;
+    Error *end_err = NULL;

     ret = g_new0(QCryptoBlockCreateOptions, 1);
     ret->format = format;
@@ -264,10 +265,9 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
         error_setg(&local_err, "Unsupported block format %d", format);
         break;
     }
-    error_propagate(errp, local_err);
-    local_err = NULL;

-    visit_end_struct(opts_get_visitor(ov), &local_err);
+    visit_end_struct(opts_get_visitor(ov), &end_err);
+    error_propagate(&local_err, end_err);

  out:
     if (local_err) {
-- 
2.5.5

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1057 bytes --]

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.

> 
> Until a later patch adds visit_check_struct(), the only safe
> approach is to use two separate error objects.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> 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 <eblake@redhat.com>
>> ---
>>  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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On 04/07/2016 09:14 AM, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]