On 01/09/2017 07:58 AM, Michal Privoznik wrote:
> The code at the very bottom of the DAC secdriver that calls
> chown() should be fine with read-only data. If something needs to
> be prepared it should have been done beforehand.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 25 ++++++++++++++++---------
> src/security/security_dac.c | 4 ++--
> src/security/security_manager.h | 2 +-
> src/storage/storage_backend.h | 2 +-
> src/storage/storage_backend_fs.c | 2 +-
> src/storage/storage_backend_gluster.c | 2 +-
> src/storage/storage_driver.c | 6 +++---
> src/storage/storage_driver.h | 4 ++--
> src/util/virstoragefile.c | 2 +-
> src/util/virstoragefile.h | 2 +-
> 10 files changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 675a4d0e7..6a45ccd9b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -329,10 +329,11 @@ qemuAutostartDomains(virQEMUDriverPtr driver)
>
>
> static int
> -qemuSecurityChownCallback(virStorageSourcePtr src,
> +qemuSecurityChownCallback(const virStorageSource *src,
> uid_t uid,
> gid_t gid)
> {
> + virStorageSourcePtr cpy = NULL;
> struct stat sb;
> int save_errno = 0;
> int ret = -1;
> @@ -355,22 +356,28 @@ qemuSecurityChownCallback(virStorageSourcePtr src,
> }
>
> return chown(src->path, uid, gid);
^^ v1 had a "ret = chown()"; goto cleanup;
hence why I suggested else logic.
Of course if you just return then the else isn't necessary. I just
envision someone coming along shortly and writing a patch to remove the
else because it's decidedly unnecessary now.
ACK - with either a ret = chown or just undoing the else now....
John
> - }
> + } else {
> + if (!(cpy = virStorageSourceCopy(src, false)))
> + goto cleanup;
>
> - /* storage file init reports errors, return -2 on failure */
> - if (virStorageFileInit(src) < 0)
> - return -2;
> + /* src file init reports errors, return -2 on failure */
> + if (virStorageFileInit(cpy) < 0) {
> + ret = -2;
> + goto cleanup;
> + }
>
> - if (virStorageFileChown(src, uid, gid) < 0) {
> - save_errno = errno;
> - goto cleanup;
> + if (virStorageFileChown(cpy, uid, gid) < 0) {
> + save_errno = errno;
> + goto cleanup;
> + }
> }
>
> ret = 0;
>
> cleanup:
> - virStorageFileDeinit(src);
> errno = save_errno;
> + virStorageFileDeinit(cpy);
> + virStorageSourceFree(cpy);
>
> return ret;
> }
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 649219e52..b6f75fe1d 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -279,8 +279,8 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
> }
>
> static int
> -virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
> - virStorageSourcePtr src,
> +virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> + const virStorageSource *src,
> const char *path,
> uid_t uid,
> gid_t gid)
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 0d22cb15f..80fceddc8 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -62,7 +62,7 @@ int virSecurityManagerStackAddNested(virSecurityManagerPtr stack,
> * @src. The callback shall return 0 on success, -1 on error and errno set (no
> * libvirt error reported) OR -2 and a libvirt error reported. */
> typedef int
> -(*virSecurityManagerDACChownCallback)(virStorageSourcePtr src,
> +(*virSecurityManagerDACChownCallback)(const virStorageSource *src,
> uid_t uid,
> gid_t gid);
>
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 28e1a6517..82fbbbf6d 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -285,7 +285,7 @@ typedef int
> int mode);
>
> typedef int
> -(*virStorageFileBackendChown)(virStorageSourcePtr src,
> +(*virStorageFileBackendChown)(const virStorageSource *src,
> uid_t uid,
> gid_t gid);
>
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index de0e8d57d..9de0c17c0 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -1600,7 +1600,7 @@ virStorageFileBackendFileAccess(virStorageSourcePtr src,
>
>
> static int
> -virStorageFileBackendFileChown(virStorageSourcePtr src,
> +virStorageFileBackendFileChown(const virStorageSource *src,
> uid_t uid,
> gid_t gid)
> {
> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
> index 8e867043e..ae0611543 100644
> --- a/src/storage/storage_backend_gluster.c
> +++ b/src/storage/storage_backend_gluster.c
> @@ -809,7 +809,7 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src)
>
>
> static int
> -virStorageFileBackendGlusterChown(virStorageSourcePtr src,
> +virStorageFileBackendGlusterChown(const virStorageSource *src,
> uid_t uid,
> gid_t gid)
> {
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 5fc706634..8f1d3f04c 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2848,7 +2848,7 @@ int storageRegister(void)
>
> /* ----------- file handlers cooperating with storage driver --------------- */
> static bool
> -virStorageFileIsInitialized(virStorageSourcePtr src)
> +virStorageFileIsInitialized(const virStorageSource *src)
> {
> return src && src->drv;
> }
> @@ -2888,7 +2888,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
> * driver to perform labelling
> */
> bool
> -virStorageFileSupportsSecurityDriver(virStorageSourcePtr src)
> +virStorageFileSupportsSecurityDriver(const virStorageSource *src)
> {
> int actualType;
> virStorageFileBackendPtr backend;
> @@ -3179,7 +3179,7 @@ virStorageFileAccess(virStorageSourcePtr src,
> * by libvirt storage backend.
> */
> int
> -virStorageFileChown(virStorageSourcePtr src,
> +virStorageFileChown(const virStorageSource *src,
> uid_t uid,
> gid_t gid)
> {
> diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h
> index 3f2549da5..682c9ff82 100644
> --- a/src/storage/storage_driver.h
> +++ b/src/storage/storage_driver.h
> @@ -44,9 +44,9 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src,
> char **buf);
> const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src);
> int virStorageFileAccess(virStorageSourcePtr src, int mode);
> -int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid);
> +int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid);
>
> -bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src);
> +bool virStorageFileSupportsSecurityDriver(const virStorageSource *src);
>
> int virStorageFileGetMetadata(virStorageSourcePtr src,
> uid_t uid, gid_t gid,
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index ce6d21388..3d4bda700 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2082,7 +2082,7 @@ virStorageSourceGetActualType(const virStorageSource *def)
>
>
> bool
> -virStorageSourceIsLocalStorage(virStorageSourcePtr src)
> +virStorageSourceIsLocalStorage(const virStorageSource *src)
> {
> virStorageType type = virStorageSourceGetActualType(src);
>
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 1f62244db..bea1c35a2 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -349,7 +349,7 @@ int virStorageSourceInitChainElement(virStorageSourcePtr newelem,
> void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def);
> void virStorageSourceClear(virStorageSourcePtr def);
> int virStorageSourceGetActualType(const virStorageSource *def);
> -bool virStorageSourceIsLocalStorage(virStorageSourcePtr src);
> +bool virStorageSourceIsLocalStorage(const virStorageSource *src);
> bool virStorageSourceIsEmpty(virStorageSourcePtr src);
> bool virStorageSourceIsBlockLocal(const virStorageSource *src);
> void virStorageSourceFree(virStorageSourcePtr def);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
