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

Saturday, 16 March 2019

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(a)redhat.com&gt;
---
 src/test/test_driver.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 0bb48b4364..8e2a9dcff6 100644
--- 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;
@@ -6508,8 +6508,6 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
                                            testDomainSnapshotDiscardAll,
                                            &rem);
         if (rem.current) {
-            if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)
-                snap->def->current = true;
             vm->current_snapshot = snap;
         }
     } else if (snap->nchildren) {
@@ -6542,8 +6540,6 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
                 if (!parentsnap) {
                     VIR_WARN("missing parent snapshot matching name
'%s'",
                              snap->def->parent);
-                } else {
-                    parentsnap->def->current = true;
                 }
             }
             vm->current_snapshot = parentsnap;
@@ -6623,12 +6619,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)
-- 
2.20.1


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005