Re: [Qemu-devel] [PATCH] qcow2: fix double-free of Qcow2DiscardRegion in qcow2_process_discards

[Kevin Wolf]

Am 11.10.2014 um 09:14 hat Zhang Haoyu geschrieben:
> In qcow2_update_snapshot_refcount -> qcow2_process_discards() -> 
> bdrv_discard()
> may free the Qcow2DiscardRegion which is referenced by "next" pointer in
> qcow2_process_discards() now, in next iteration, d = next, so g_free(d)
> will double-free this Qcow2DiscardRegion.
> 
> qcow2_snapshot_delete
> |- qcow2_update_snapshot_refcount
> |-- qcow2_process_discards
> |--- bdrv_discard
> |---- aio_poll
> |----- aio_dispatch
> |------ bdrv_co_io_em_complete
> |------- qemu_coroutine_enter(co->coroutine, NULL); <=== coroutine entry is 
> bdrv_co_do_rw
> |--- g_free(d) <== free first Qcow2DiscardRegion is okay
> |--- d = next;  <== this set is done in QTAILQ_FOREACH_SAFE() macro.
> |--- g_free(d);  <== double-free will happen if during previous iteration, 
> bdrv_discard had free this object.

Do you have a reproducer for this or did code review lead you to this?

At the moment I can't see how bdrv_discard(bs->file) could ever free a
Qcow2DiscardRegion of bs, as it's working on a completely different
BlockDriverState (which usually won't even be a qcow2 one).

> bdrv_co_do_rw
> |- bdrv_co_do_writev
> |-- bdrv_co_do_pwritev
> |--- bdrv_aligned_pwritev
> |---- qcow2_co_writev
> |----- qcow2_alloc_cluster_link_l2
> |------ qcow2_free_any_clusters
> |------- qcow2_free_clusters
> |-------- update_refcount
> |--------- qcow2_process_discards
> |---------- g_free(d)  <== In next iteration, this Qcow2DiscardRegion will be 
> double-free.

This shouldn't happen in a nested call either, as s->lock can't be taken
recursively.

Kevin