[libvirt] [PATCH 2/6] security_dac: Resolve virSecurityDACSetOwnershipInternal const correctness
John Ferlan
jferlan at redhat.com
Sat Jan 7 14:01:46 UTC 2017
On 12/19/2016 10:57 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 at redhat.com>
> ---
> src/qemu/qemu_driver.c | 28 ++++++++++++++++++++--------
> 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, 33 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1a464337e..f68b6ff30 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 *storage,
> uid_t uid,
> gid_t gid)
> {
> + virStorageSourcePtr src = NULL;
^^^
> struct stat sb;
> int save_errno = 0;
> int ret = -1;
> @@ -340,26 +341,35 @@ qemuSecurityChownCallback(virStorageSourcePtr src,
> if (!virStorageFileSupportsSecurityDriver(src))
src is NULL here...
> return 0;
>
> + if (!(src = virStorageSourceCopy(storage, false)))
> + goto cleanup;
> +
If the copy is only needed for remote, why not special case that...
> if (virStorageSourceIsLocalStorage(src)) {
> /* use direct chmod for local files so that the file doesn't
> * need to be initialized */
> - if (!src->path)
> - return 0;
> + if (!src->path) {
> + ret = 0;
> + goto cleanup;
> + }
>
> if (stat(src->path, &sb) >= 0) {
> if (sb.st_uid == uid &&
> sb.st_gid == gid) {
> /* It's alright, there's nothing to change anyway. */
> - return 0;
> + ret = 0;
> + goto cleanup;
> }
> }
>
> - return chown(src->path, uid, gid);
> + ret = chown(src->path, uid, gid);
> + goto cleanup;
> }
Theoretically this could be an } else {
then allocate source "cpy" here with the explanation that FileInit will
create/write src->drv for remote usage... Use "cpy" instead of 'src' for
the remote copy
>
> /* storage file init reports errors, return -2 on failure */
> - if (virStorageFileInit(src) < 0)
> - return -2;
> + if (virStorageFileInit(src) < 0) {
> + ret = -2;
> + goto cleanup;
> + }
>
> if (virStorageFileChown(src, uid, gid) < 0) {
> save_errno = errno;
> @@ -370,7 +380,9 @@ qemuSecurityChownCallback(virStorageSourcePtr src,
>
> cleanup:
> virStorageFileDeinit(src);
> - errno = save_errno;
> + virStorageSourceFree(src);
> + if (save_errno)
> + errno = save_errno;
No reason for if (save_errno) - besides it probably should be "if
(save_errno == 0)"
John
>
> 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 a79acc6f0..1ac07dbf2 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2832,7 +2832,7 @@ int storageRegister(void)
>
> /* ----------- file handlers cooperating with storage driver --------------- */
> static bool
> -virStorageFileIsInitialized(virStorageSourcePtr src)
> +virStorageFileIsInitialized(const virStorageSource *src)
> {
> return src && src->drv;
> }
> @@ -2872,7 +2872,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
> * driver to perform labelling
> */
> bool
> -virStorageFileSupportsSecurityDriver(virStorageSourcePtr src)
> +virStorageFileSupportsSecurityDriver(const virStorageSource *src)
> {
> int actualType;
> virStorageFileBackendPtr backend;
> @@ -3163,7 +3163,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);
>
More information about the libvir-list
mailing list