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

[Ján Tomko]

On Wed, Mar 20, 2019 at 12:40:50AM -0500, 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 | 19 +++++--------------
> 1 file changed, 5 insertions(+), 14 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190320/1e60942d/attachment-0001.sig>