Re: [PATCH v2 1/5] security_dac: Resolve virSecurityDACSetOwnershipInternal const correctness

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux