[libvirt] [PATCH] test: Avoid use-after-free on virDomainSnapshotDelete

[Michal Privoznik]

On 3/17/19 5:13 AM, Eric Blake wrote:
> The following virsh command was triggering a use-after-free:
> 
> $ virsh -c test:///default '
>    snapshot-create-as test s1
>    snapshot-create-as test s2
>    snapshot-delete --children-only test s1
>    snapshot-current --name test'
> Domain snapshot s1 created
> Domain snapshot s2 created
> Domain snapshot s1 children deleted
> 
> error: name in virGetDomainSnapshot must not be NULL
> 
> I got lucky on that run - although the error message is quite
> unexpected.  On other runs, I was able to get a core dump, and
> valgrind confirms there is a definitive problem.
> 
> The culprit? We were inconsistent about whether we set
> vm->current_snapshot, snap->def->current, or both when updating how
> the current snapshot was being tracked.  As a result, deletion did not
> see that snapshot s2 was previously current, and failed to update
> vm->current_snapshot, so that the next API using the current snapshot
> failed because it referenced stale memory for the now-gone s2 (instead
> of the intended s1).
> 
> The test driver code was copied from the qemu code (which DOES track
> both pieces of state everywhere), but was purposefully simplified
> because the test driver does not have to write persistent snapshot
> state to the file system.  But when you realize that the only reason
> snap->def->current needs to exist is when writing out one file per
> snapshot for qemu, it's just as easy to state that the test driver
> never has to mess with the field (rather than chasing down which
> places forgot to set the field), and have vm->current_snapshot be the
> sole source of truth in the test driver.
> 
> Ideally, I'd get rid of the 'current' member in virDomainSnapshotDef,
> as well as the 'current_snapshot' member in virDomainDef, and instead
> track the current member in virDomainSnapshotObjList, coupled with
> writing ALL snapshot state for qemu in a single file (where I can use
> <snapshots current='...'> as a wrapper, rather than
> VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL to output <current>1</current> XML
> on a per-snapshot file basis).  But that's a bigger change, so for now
> I'm just patching things to avoid the test driver segfault.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>   src/test/test_driver.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)

I'm unable to apply this after you've merged your other patches. Can you 
please rebase and resend?

Thanks,
Michal