Re: [Qemu-block] [PATCH v9 07/20] block: deprecate "encryption=on" in favor of "encrypt.format=aes"

[Daniel P. Berrange]

On Tue, Jun 20, 2017 at 04:07:49PM +0200, Alberto Garcia wrote:
> On Tue 20 Jun 2017 02:02:06 PM CEST, Daniel P. Berrange wrote:
> >> > +    if (encryptfmt) {
> >> > +        buf = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT);
> >> > +        if (buf != NULL) {
> >> > +            g_free(buf);
> >> 
> >> If you use qemu_opt_get() instead then you don't need "buf" at all,
> >> do you?
> >
> > IIRC, we needed to delete the option from opts, otherwise something
> > will later complain that there are opts that are not consumed.
> 
> I don't see how, since once you reached this point it means that you
> cannot create the image because the options are already wrong.

Ah yes, that is of course true.

> Anyway, there's a more important problem with this patch that I just
> noticed: in this scenario you're freeing 'buf' twice, first in the snip
> I quoted above, and then at the end of qcow2_create().
> 
> qcow_create() from qcow.c is safe, but perhaps it's clearer if you put
> the declaration of the 'buf' variable inside the 'if (encryptfmt)'
> block.
> 
> That assuming that you really need to use qemu_opt_get_del() there. With
> the double-free bug fixed, I don't see any difference when using
> qemu_opt_get_del() vs qemu_opt_get().

I'll switch to qemu_opt_get as you suggest.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|