test: Avoid use-after-free on virDomainSnapshotDelete

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@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 0bb48b4364ba2e5576f6b2d4c104d84ca8f454ef..bd0a14114ed3b2296da7688ab470a2d6d5e6e0bc 100644 (file)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1,7 +1,7 @@
 /*
  * test_driver.c: A "mock" hypervisor for use by application unit tests
  *
- * Copyright (C) 2006-2015 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -6441,7 +6441,7 @@ testDomainSnapshotDiscardAll(void *payload,
     virDomainSnapshotObjPtr snap = payload;
     testSnapRemoveDataPtr curr = data;
 
-    if (snap->def->current)
+    if (curr->vm->current_snapshot == snap)
         curr->current = true;
     virDomainSnapshotObjListRemove(curr->vm->snapshots, snap);
     return 0;
@@ -6507,11 +6507,8 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
         virDomainSnapshotForEachDescendant(snap,
                                            testDomainSnapshotDiscardAll,
                                            &rem);
-        if (rem.current) {
-            if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)
-                snap->def->current = true;
+        if (rem.current)
             vm->current_snapshot = snap;
-        }
     } else if (snap->nchildren) {
         testSnapReparentData rep;
         rep.parent = snap->parent;
@@ -6539,12 +6536,9 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
             if (snap->def->parent) {
                 parentsnap = virDomainSnapshotFindByName(vm->snapshots,
                                                          snap->def->parent);
-                if (!parentsnap) {
+                if (!parentsnap)
                     VIR_WARN("missing parent snapshot matching name '%s'",
                              snap->def->parent);
-                } else {
-                    parentsnap->def->current = true;
-                }
             }
             vm->current_snapshot = parentsnap;
         }
@@ -6623,12 +6617,9 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
     }
 
 
-    if (vm->current_snapshot) {
-        vm->current_snapshot->def->current = false;
+    if (vm->current_snapshot)
         vm->current_snapshot = NULL;
-    }
 
-    snap->def->current = true;
     config = virDomainDefCopy(snap->def->dom, privconn->caps,
                               privconn->xmlopt, NULL, true);
     if (!config)