On 08/27/2018 04:08 AM, Michal Privoznik wrote:
You have nothing to say here, wow we got this far and your speechless...
Or your fingers were just really tired from all those patches!
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/security/security_dac.c | 52 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 2115e00e07..818548dd22 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -638,6 +638,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> struct stat sb;
> int rc;
> + bool locked = false;
> + int ret = -1;
>
> if (!path && src && src->path &&
> virStorageSourceIsLocalStorage(src))
> @@ -657,14 +659,28 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> return -1;
> }
>
> + if (!S_ISDIR(sb.st_mode)) {
Seems we leave ourselves open for a few other odd combinations we don't
want... Should this be restricted to certain things like S_ISREG?
> + if (virSecurityManagerMetadataLock(mgr, path) < 0)
> + return -1;
> + locked = true;
> + }
> +
> if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0)
> - return -1;
> + goto cleanup;
> }
>
> VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> NULLSTR(src ? src->path : path), (long)uid, (long)gid);
>
> - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + if (locked &&
> + virSecurityManagerMetadataUnlock(mgr, path) < 0)
> + VIR_WARN("Unable to unlock metadata on %s", path);
> + return ret;
> }
>
>
> @@ -677,6 +693,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> int rv;
> uid_t uid = 0; /* By default return to root:root */
> gid_t gid = 0;
> + struct stat sb;
> + bool locked;
Coverity says, please initialize this to false since it's used in cleanup
> + int ret = -1;
>
> if (!path && src && src->path &&
> virStorageSourceIsLocalStorage(src))
> @@ -691,17 +710,38 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
> return 0;
>
> if (path) {
> + if (stat(path, &sb) < 0) {
> + virReportSystemError(errno, _("unable to stat: %s"), path);
> + return -1;
> + }
> +
> + if (!S_ISDIR(sb.st_mode)) {
Similar S_ISREG
I'll wait for when there's an update before R_By, but I see nothing
inherently wrong that sticks out - I'm just in search of liquid
refreshment right now, so this is as good as it gets.
John
> + if (virSecurityManagerMetadataLock(mgr, path) < 0)
> + return -1;
> + locked = true;
> + }
> +
> rv = virSecurityDACRecallLabel(priv, path, &uid, &gid);
> if (rv < 0)
> - return -1;
> - if (rv > 0)
> - return 0;
> + goto cleanup;
> + if (rv > 0) {
> + ret = 0;
> + goto cleanup;
> + }
> }
>
> VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld",
> NULLSTR(src ? src->path : path), (long)uid, (long)gid);
>
> - return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + if (locked &&
> + virSecurityManagerMetadataUnlock(mgr, path) < 0)
> + VIR_WARN("Unable to unlock metadata on %s", path);
> + return ret;
> }
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
