Re: [libvirt] [PATCH] storage: Don't dereference driver object if virStorageSource is not initialized

On 12/06/2017 10:28 AM, Peter Krempa wrote: > virStorageFileReportBrokenChain uses data from the driver private data > pointer to print the user and group. This would lead to a crash in call > paths where we did not initialize the storage backend as recently added > in commit 24e47ee2b93 to qemuDomainDetermineDiskChain. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1522682 > --- > > Once pushed I'll also backport it to a maint branch of the last release > if that's desired. > > src/storage/storage_source.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c > index 4586ef4ad4..a5df84bae3 100644 > --- a/src/storage/storage_source.c > +++ b/src/storage/storage_source.c > @@ -419,19 +419,33 @@ virStorageFileReportBrokenChain(int errcode, > virStorageSourcePtr src, > virStorageSourcePtr parent) > { > - unsigned int access_user = src->drv->uid; > - unsigned int access_group = src->drv->gid; > - > - if (src == parent) { > - virReportSystemError(errcode, > - _("Cannot access storage file '%s' " > - "(as uid:%u, gid:%u)"), > - src->path, access_user, access_group); > + > + if (src->drv) { > + unsigned int access_user = src->drv->uid; > + unsigned int access_group = src->drv->gid; > + > + if (src == parent) { > + virReportSystemError(errcode, > + _("Cannot access storage file '%s' " > + "(as uid:%u, gid:%u)"), > + src->path, access_user, access_group); > + } else { > + virReportSystemError(errcode, > + _("Cannot access backing file '%s' " > + "of storage file '%s' (as uid:%u, gid:%u)"), > + src->path, parent->path, access_user, access_group); > + } > } else { > - virReportSystemError(errcode, > - _("Cannot access backing file '%s' " > - "of storage file '%s' (as uid:%u, gid:%u)"), > - src->path, parent->path, access_user, access_group); > + if (src == parent) { > + virReportSystemError(errcode, > + _("Cannot access storage file '%s' "), ^ Not that it matters much because it probably won't be seen, but you could drop the trailing space.

Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;

John > + src->path); > + } else { > + virReportSystemError(errcode, > + _("Cannot access backing file '%s' " > + "of storage file '%s'"), > + src->path, parent->path); > + } > } > } >