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 :|
