[Qemu-devel] [PATCH] block/crypto: Simplify block_crypto_co_create_opts_luks to avoid a memory leak

[Qemu-devel] [PATCH] block/crypto: Simplify block_crypto_co_create_opts_luks to avoid a memory leak

[Philippe Mathieu-Daudé]

After 1ec4f4160a1 Coverity reported:

  Variable cryptoopts going out of scope leaks the storage it points to.

Fixes: Coverity CID 1393782 (Resource leak)
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
I think this check is superfluous but I respected the previous code:

     ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
     if (ret > 0) {
         ret = 0;
     }

 block/crypto.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 994172a3de..d4d2c6c511 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -545,33 +545,36 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
     create_opts = block_crypto_create_opts_init(cryptoopts, errp);
     if (!create_opts) {
         ret = -EINVAL;
-        goto fail;
+        goto cleanup_cryptoopts;
     }
 
     /* Create protocol layer */
     ret = bdrv_create_file(filename, opts, errp);
     if (ret < 0) {
-        return ret;
+        goto cleanup_create_opts;
     }
 
     bs = bdrv_open(filename, NULL, NULL,
                    BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (!bs) {
         ret = -EINVAL;
-        goto fail;
+        goto cleanup_create_opts;
     }
 
     /* Create format layer */
     ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
-    if (ret < 0) {
-        goto fail;
+    if (ret > 0) {
+        ret = 0;
     }
 
-    ret = 0;
-fail:
     bdrv_unref(bs);
+
+cleanup_create_opts:
     qapi_free_QCryptoBlockCreateOptions(create_opts);
+
+cleanup_cryptoopts:
     qobject_unref(cryptoopts);
+
     return ret;
 }
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH] block/crypto: Simplify block_crypto_co_create_opts_luks to avoid a memory leak

[Kevin Wolf]

Am 04.07.2018 um 17:02 hat Philippe Mathieu-Daudé geschrieben:
> After 1ec4f4160a1 Coverity reported:
> 
>   Variable cryptoopts going out of scope leaks the storage it points to.
> 
> Fixes: Coverity CID 1393782 (Resource leak)
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I already sent a much simpler fix:
[PATCH] block/crypto: Fix memory leak in create error path

The only thing that is needed is replacing the return with a goto.
Splitting it in three different error paths is unnecessary because the
cleanup function handle NULL values just fine.

> I think this check is superfluous but I respected the previous code:
> 
>      ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
>      if (ret > 0) {
>          ret = 0;
>      }

It is wrong, too. The old code keep the error code, goto fail skipped
the ret = 0.

Kevin

Re: [Qemu-devel] [PATCH] block/crypto: Simplify block_crypto_co_create_opts_luks to avoid a memory leak

[Philippe Mathieu-Daudé]

On 07/05/2018 07:20 AM, Kevin Wolf wrote:
> Am 04.07.2018 um 17:02 hat Philippe Mathieu-Daudé geschrieben:
>> After 1ec4f4160a1 Coverity reported:
>>
>>   Variable cryptoopts going out of scope leaks the storage it points to.
>>
>> Fixes: Coverity CID 1393782 (Resource leak)
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I already sent a much simpler fix:
> [PATCH] block/crypto: Fix memory leak in create error path

Oh OK I searched a bit but missed it.

> The only thing that is needed is replacing the return with a goto.
> Splitting it in three different error paths is unnecessary because the
> cleanup function handle NULL values just fine.

OK, good to know.

>> I think this check is superfluous but I respected the previous code:
>>
>>      ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
>>      if (ret > 0) {
>>          ret = 0;
>>      }
> 
> It is wrong, too. The old code keep the error code, goto fail skipped
> the ret = 0.

So this is not particularly wrong but as superfluous as the current use :)

ret = 0 is only useful if block_crypto_co_create_generic() returned a
value > 0, which seems unlikely.

Regards,

Phil.

Re: [Qemu-devel] [PATCH] block/crypto: Simplify block_crypto_co_create_opts_luks to avoid a memory leak

[Kevin Wolf]

Am 05.07.2018 um 18:04 hat Philippe Mathieu-Daudé geschrieben:
> On 07/05/2018 07:20 AM, Kevin Wolf wrote:
> > Am 04.07.2018 um 17:02 hat Philippe Mathieu-Daudé geschrieben:
> >> After 1ec4f4160a1 Coverity reported:
> >>
> >>   Variable cryptoopts going out of scope leaks the storage it points to.
> >>
> >> Fixes: Coverity CID 1393782 (Resource leak)
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > 
> > I already sent a much simpler fix:
> > [PATCH] block/crypto: Fix memory leak in create error path
> 
> Oh OK I searched a bit but missed it.
> 
> > The only thing that is needed is replacing the return with a goto.
> > Splitting it in three different error paths is unnecessary because the
> > cleanup function handle NULL values just fine.
> 
> OK, good to know.
> 
> >> I think this check is superfluous but I respected the previous code:
> >>
> >>      ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
> >>      if (ret > 0) {
> >>          ret = 0;
> >>      }
> > 
> > It is wrong, too. The old code keep the error code, goto fail skipped
> > the ret = 0.
> 
> So this is not particularly wrong but as superfluous as the current use :)
> 
> ret = 0 is only useful if block_crypto_co_create_generic() returned a
> value > 0, which seems unlikely.

Sorry, yes, you're right. I read 'if (ret < 0)' in your patch.

The reason for the seemingly superfluous error path is that you can add
new code behind it without having to modify the existing code.

Kevin