From nobody Mon Feb 9 17:22:03 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) client-ip=209.132.183.28; envelope-from=libvir-list-bounces@redhat.com; helo=mx1.redhat.com; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.zohomail.com with SMTPS id 1553060495919936.6015452381857; Tue, 19 Mar 2019 22:41:35 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9B5AE811A9; Wed, 20 Mar 2019 05:41:33 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 76D111018A36; Wed, 20 Mar 2019 05:41:33 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id 3D54F3FB12; Wed, 20 Mar 2019 05:41:33 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id x2K5f92a017170 for ; Wed, 20 Mar 2019 01:41:09 -0400 Received: by smtp.corp.redhat.com (Postfix) id 0056517F8D; Wed, 20 Mar 2019 05:41:09 +0000 (UTC) Received: from blue.redhat.com (ovpn-116-65.phx2.redhat.com [10.3.116.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id 936FA17795; Wed, 20 Mar 2019 05:41:08 +0000 (UTC) From: Eric Blake To: libvir-list@redhat.com Date: Wed, 20 Mar 2019 00:40:50 -0500 Message-Id: <20190320054105.17689-2-eblake@redhat.com> In-Reply-To: <20190320054105.17689-1-eblake@redhat.com> References: <20190320054105.17689-1-eblake@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-loop: libvir-list@redhat.com Cc: jtomko@redhat.com Subject: [libvirt] [PATCH 01/16] test: Avoid use-after-free on virDomainSnapshotDelete X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 20 Mar 2019 05:41:34 +0000 (UTC) Content-Type: text/plain; charset="utf-8" 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 as a wrapper, rather than VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL to output 1 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 Reviewed-by: J=C3=A1n Tomko --- src/test/test_driver.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0bb48b4364..bd0a14114e 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 =3D payload; testSnapRemoveDataPtr curr =3D data; - if (snap->def->current) + if (curr->vm->current_snapshot =3D=3D snap) curr->current =3D true; virDomainSnapshotObjListRemove(curr->vm->snapshots, snap); return 0; @@ -6507,11 +6507,8 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapsh= ot, virDomainSnapshotForEachDescendant(snap, testDomainSnapshotDiscardAll, &rem); - if (rem.current) { - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) - snap->def->current =3D true; + if (rem.current) vm->current_snapshot =3D snap; - } } else if (snap->nchildren) { testSnapReparentData rep; rep.parent =3D snap->parent; @@ -6539,12 +6536,9 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapsh= ot, if (snap->def->parent) { parentsnap =3D 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 =3D true; - } } vm->current_snapshot =3D parentsnap; } @@ -6623,12 +6617,9 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snap= shot, } - if (vm->current_snapshot) { - vm->current_snapshot->def->current =3D false; + if (vm->current_snapshot) vm->current_snapshot =3D NULL; - } - snap->def->current =3D true; config =3D virDomainDefCopy(snap->def->dom, privconn->caps, privconn->xmlopt, NULL, true); if (!config) --=20 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list