Re: [libvirt] [PATCH v3 08/18] snapshot: Avoid latent use-after-free when cleaning snapshots

On 3/4/19 10:34 PM, Eric Blake wrote: > Right now, the only callers of qemuDomainSnapshotDiscardAllMetadata() > are right before freeing the virDomainSnapshotObjList, so it did not > matter if the list's metaroot (which points to all the defined root > snapshots) is left inconsistent. But an upcoming patch will want to > clear all snapshots if a bulk redefine fails partway through, in > which case things must be reset. Make this work by teaching the > existing virDomainSnapshotUpdateRelations() to be safe regardless of > the incoming state of the metaroot (since we don't want to leak that > internal detail into qemu code), then fixing the qemu code to use > it after deleting all snapshots. > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > src/conf/snapshot_conf.c | 7 +++++-- > src/qemu/qemu_domain.c | 4 ++++ > 2 files changed, 9 insertions(+), 2 deletions(-) > I have some random personal grumbling about the lack of comments for existing code. What's described in this patch as being done would seem correct, but my knowledge of snapshot's is 'cursory'.

> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c > index 206b05c172..386ec82d15 100644 > --- a/src/conf/snapshot_conf.c > +++ b/src/conf/snapshot_conf.c > @@ -1209,13 +1209,16 @@ virDomainSnapshotSetRelations(void *payload, > } > > /* Populate parent link and child count of all snapshots, with all > - * relations starting as 0/NULL. Return 0 on success, -1 if a parent > - * is missing or if a circular relationship was requested. */ > + * assigned defs having relations starting as 0/NULL. Return 0 on > + * success, -1 if a parent is missing or if a circular relationship > + * was requested. */ ^^ nit: there's an extra space here

> +++ b/src/qemu/qemu_domain.c > @@ -8625,6 +8625,10 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, > rem.err = 0; > virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, > &rem); > + if (rem.current) > + vm->current_snapshot = NULL; If rem.err != 0, is this still something that should be done