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@redhat.com> --- 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; }