[PATCH 00/19] security_selinux: Refactor temporary variable cleanup
Some cleanups resutling from analyzing some selinux code. Peter Krempa (19): security/security_driver.c/h: Fix function header formatting virSecuritySELinuxTransactionRun: Refactor cleanup virSecuritySELinuxMCSFind: Refactor variable clearing virSecuritySELinuxRestoreFileLabel: Refactor cleanup virSecuritySELinux(Set|Restore)TPMFileLabel: Automatically free 'cancel_path' virSecuritySELinuxRestoreFileLabels: Refactor variable freeing virSecuritySELinux(Set|Restore)HostdevCapsLabel: Refactor cleanup virSecuritySELinux(Set|Restore)ChardevLabel: Refactor cleanup virSecuritySELinuxSet(Daemon)SocketLabel: Refactor cleanup virSecuritySELinuxSetTapFDLabel: Refactor cleanup security_selinux: Declare internal autoptr cleanup helper for 'context_t' virSecuritySELinuxContextAddRange: Refactor cleanup of 'context_t' virSecuritySELinuxGenNewContext: Refactor cleanup virSecuritySELinuxReserveLabel: Refactor cleanup virSecuritySELinuxGetProcessLabel: Automatically free 'ctx' temp variable virSecuritySELinuxSetFilecon: Automatically free 'econ' temp variable virSecuritySELinuxRestoreInputLabel: Return values directly virSecuritySELinuxGenImageLabel: Refactor cleanup virSecuritySELinuxGetSecurityMountOptions: refactor printing src/security/security_driver.c | 5 +- src/security/security_driver.h | 5 +- src/security/security_selinux.c | 347 +++++++++++++------------------- 3 files changed, 149 insertions(+), 208 deletions(-) -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Fix the misaligned arguments by switching over to modern style. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_driver.c | 5 +++-- src/security/security_driver.h | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/security/security_driver.c b/src/security/security_driver.c index ae11f595eb..4cb270e836 100644 --- a/src/security/security_driver.c +++ b/src/security/security_driver.c @@ -45,8 +45,9 @@ static virSecurityDriver *security_drivers[] = { &virSecurityDriverNop, /* Must always be last, since it will always probe */ }; -virSecurityDriver *virSecurityDriverLookup(const char *name, - const char *virtDriver) +virSecurityDriver * +virSecurityDriverLookup(const char *name, + const char *virtDriver) { virSecurityDriver *drv = NULL; size_t i; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 5ab4d6ca1e..b8c5b416e3 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -245,5 +245,6 @@ struct _virSecurityDriver { virSecurityDomainRestoreNetdevLabel domainRestoreSecurityNetdevLabel; }; -virSecurityDriver *virSecurityDriverLookup(const char *name, - const char *virtDriver); +virSecurityDriver * +virSecurityDriverLookup(const char *name, + const char *virtDriver); -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free 'paths' and remove temporary variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 64e7f41ce0..0326073810 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -282,11 +282,10 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED, { virSecuritySELinuxContextList *list = opaque; virSecurityManagerMetadataLockState *state; - const char **paths = NULL; + g_autofree const char **paths = NULL; size_t npaths = 0; size_t i; int rv; - int ret = -1; if (list->lock) { paths = g_new0(const char *, list->nItems); @@ -303,7 +302,7 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED, list->sharedFilesystems, paths, npaths, list->lockMetadataException))) - goto cleanup; + return -1; for (i = 0; i < list->nItems; i++) { virSecuritySELinuxContextItem *item = list->items[i]; @@ -357,12 +356,9 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED, virSecurityManagerMetadataUnlock(list->manager, &state); if (rv < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(paths); - return ret; + return 0; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Use automatic freeing for 'mcs' and adjust the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0326073810..28ac136635 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -398,7 +398,6 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr, { virSecuritySELinuxData *data = virSecurityManagerGetPrivateData(mgr); int catRange; - char *mcs = NULL; /* +1 since virRandomInt range is exclusive of the upper bound */ catRange = (catMax - catMin) + 1; @@ -416,6 +415,7 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr, for (;;) { int c1 = virRandomInt(catRange); int c2 = virRandomInt(catRange); + g_autofree char *mcs = NULL; VIR_DEBUG("Try cat %s:c%d,c%d", sens, c1 + catMin, c2 + catMin); @@ -439,12 +439,10 @@ virSecuritySELinuxMCSFind(virSecurityManager *mgr, } if (virHashLookup(data->mcs, mcs) == NULL) - break; - - VIR_FREE(mcs); + return g_steal_pointer(&mcs); } - return mcs; + return NULL; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free 'fcon' and 'newpath' and remove the 'cleanup' label and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 28ac136635..75ad5803a9 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1526,10 +1526,9 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, { bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; - char *fcon = NULL; - char *newpath = NULL; + g_autofree char *fcon = NULL; + g_autofree char *newpath = NULL; int rc; - int ret = -1; /* Some paths are auto-generated, so let's be safe here and do * nothing if nothing is needed. @@ -1544,15 +1543,14 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, if (virFileResolveLink(path, &newpath) < 0) { VIR_WARN("cannot resolve symlink %s: %s", path, g_strerror(errno)); - goto cleanup; + return -1; } if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, recall, true)) < 0) { - goto cleanup; + return -1; } else if (rc > 0) { - ret = 0; - goto cleanup; + return 0; } if (recall) { @@ -1560,10 +1558,9 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, if (rc == -2) { /* Not supported. Lookup the default label below. */ } else if (rc < 0) { - goto cleanup; + return -1; } else if (rc > 0) { - ret = 0; - goto cleanup; + return 0; } } @@ -1571,7 +1568,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, if (stat(newpath, &buf) != 0) { VIR_WARN("cannot stat %s: %s", newpath, g_strerror(errno)); - goto cleanup; + return -1; } if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { @@ -1579,19 +1576,14 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, * which makes this an expected non error */ VIR_WARN("cannot lookup default selinux label for %s", newpath); - ret = 0; - goto cleanup; + return 0; } } if (virSecuritySELinuxSetFileconImpl(newpath, fcon, privileged) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - freecon(fcon); - VIR_FREE(newpath); - return ret; + return 0; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 75ad5803a9..f042da4a13 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1737,7 +1737,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManager *mgr, { int rc; virSecurityLabelDef *seclabel; - char *cancel_path; + g_autofree char *cancel_path = NULL; const char *tpmdev; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); @@ -1755,7 +1755,6 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManager *mgr, rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, seclabel->imagelabel, false); - VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); return -1; @@ -1786,7 +1785,7 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManager *mgr, { int rc = 0; virSecurityLabelDef *seclabel; - char *cancel_path; + g_autofree char *cancel_path = NULL; const char *tpmdev; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); @@ -1801,7 +1800,6 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManager *mgr, if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0) rc = -1; - VIR_FREE(cancel_path); } break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Declare 'filename' inside the loop that is using it and use automatic freeing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f042da4a13..854e46954e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3698,7 +3698,6 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManager *mgr, { int ret = 0; struct dirent *ent; - char *filename = NULL; g_autoptr(DIR) dir = NULL; if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true))) @@ -3711,9 +3710,8 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManager *mgr, return -1; while ((ret = virDirRead(dir, &ent, path)) > 0) { - filename = g_strdup_printf("%s/%s", path, ent->d_name); + g_autofree char *filename = g_strdup_printf("%s/%s", path, ent->d_name); ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, true); - VIR_FREE(filename); if (ret < 0) break; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Declare 'path' only in blocks where it's used and autofree it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 854e46954e..2cdc6bf954 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2333,7 +2333,6 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManager *mgr, { int ret = -1; virSecurityLabelDef *secdef; - char *path; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) @@ -2341,6 +2340,7 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManager *mgr, switch (dev->source.caps.type) { case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: { + g_autofree char *path = NULL; if (vroot) { path = g_strdup_printf("%s/%s", vroot, dev->source.caps.u.storage.block); @@ -2348,11 +2348,11 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManager *mgr, path = g_strdup(dev->source.caps.u.storage.block); } ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel, true); - VIR_FREE(path); break; } case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC: { + g_autofree char *path = NULL; if (vroot) { path = g_strdup_printf("%s/%s", vroot, dev->source.caps.u.misc.chardev); @@ -2360,7 +2360,6 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManager *mgr, path = g_strdup(dev->source.caps.u.misc.chardev); } ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel, true); - VIR_FREE(path); break; } @@ -2562,10 +2561,10 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManager *mgr, const char *vroot) { int ret = -1; - char *path; switch (dev->source.caps.type) { case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: { + g_autofree char *path = NULL; if (vroot) { path = g_strdup_printf("%s/%s", vroot, dev->source.caps.u.storage.block); @@ -2573,11 +2572,11 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManager *mgr, path = g_strdup(dev->source.caps.u.storage.block); } ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); - VIR_FREE(path); break; } case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC: { + g_autofree char *path = NULL; if (vroot) { path = g_strdup_printf("%s/%s", vroot, dev->source.caps.u.misc.chardev); @@ -2585,7 +2584,6 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManager *mgr, path = g_strdup(dev->source.caps.u.misc.chardev); } ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); - VIR_FREE(path); break; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Declare 'in'/'out' only in the bocks which use them and automatically free them. Since cleanup section was removed we don't need a 'ret' variable any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 87 +++++++++++++++------------------ 1 file changed, 40 insertions(+), 47 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2cdc6bf954..25f0c45955 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2667,8 +2667,6 @@ virSecuritySELinuxSetChardevLabel(virSecurityManager *mgr, virSecurityLabelDef *seclabel; virSecurityDeviceLabelDef *chr_seclabel = NULL; char *imagelabel = NULL; - char *in = NULL, *out = NULL; - int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (!seclabel || !seclabel->relabel) @@ -2693,10 +2691,12 @@ virSecuritySELinuxSetChardevLabel(virSecurityManager *mgr, switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecuritySELinuxSetFilecon(mgr, - dev_source->data.file.path, - imagelabel, - true); + if (virSecuritySELinuxSetFilecon(mgr, + dev_source->data.file.path, + imagelabel, + true) < 0) + return -1; + break; case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -2710,37 +2710,35 @@ virSecuritySELinuxSetChardevLabel(virSecurityManager *mgr, dev_source->data.nix.path, imagelabel, true) < 0) - goto done; + return -1; } - ret = 0; + break; - case VIR_DOMAIN_CHR_TYPE_PIPE: - in = g_strdup_printf("%s.in", dev_source->data.file.path); - out = g_strdup_printf("%s.out", dev_source->data.file.path); + case VIR_DOMAIN_CHR_TYPE_PIPE: { + g_autofree char *in = g_strdup_printf("%s.in", dev_source->data.file.path); + g_autofree char *out = g_strdup_printf("%s.out", dev_source->data.file.path); if (virFileExists(in) && virFileExists(out)) { if ((virSecuritySELinuxSetFilecon(mgr, in, imagelabel, true) < 0) || - (virSecuritySELinuxSetFilecon(mgr, out, imagelabel, true) < 0)) { - goto done; - } - } else if (virSecuritySELinuxSetFilecon(mgr, - dev_source->data.file.path, - imagelabel, - true) < 0) { - goto done; + (virSecuritySELinuxSetFilecon(mgr, out, imagelabel, true) < 0)) + return -1; + + } else { + if (virSecuritySELinuxSetFilecon(mgr, + dev_source->data.file.path, + imagelabel, + true) < 0) + return -1; } - ret = 0; + break; + } default: - ret = 0; break; } - done: - VIR_FREE(in); - VIR_FREE(out); - return ret; + return 0; } static int @@ -2752,8 +2750,6 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr, { virSecurityLabelDef *seclabel; virSecurityDeviceLabelDef *chr_seclabel = NULL; - char *in = NULL, *out = NULL; - int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (!seclabel || !seclabel->relabel) @@ -2775,8 +2771,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr, if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, true) < 0) - goto done; - ret = 0; + return -1; + break; case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -2784,36 +2780,33 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr, if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.nix.path, true) < 0) - goto done; + return -1; } - ret = 0; + break; - case VIR_DOMAIN_CHR_TYPE_PIPE: - out = g_strdup_printf("%s.out", dev_source->data.file.path); - in = g_strdup_printf("%s.in", dev_source->data.file.path); + case VIR_DOMAIN_CHR_TYPE_PIPE: { + g_autofree char *out = g_strdup_printf("%s.out", dev_source->data.file.path); + g_autofree char *in = g_strdup_printf("%s.in", dev_source->data.file.path); if (virFileExists(in) && virFileExists(out)) { if ((virSecuritySELinuxRestoreFileLabel(mgr, out, true) < 0) || - (virSecuritySELinuxRestoreFileLabel(mgr, in, true) < 0)) { - goto done; - } - } else if (virSecuritySELinuxRestoreFileLabel(mgr, - dev_source->data.file.path, - true) < 0) { - goto done; + (virSecuritySELinuxRestoreFileLabel(mgr, in, true) < 0)) + return -1; + + } else { + if (virSecuritySELinuxRestoreFileLabel(mgr, + dev_source->data.file.path, + true) < 0) + return -1; } - ret = 0; + } break; default: - ret = 0; break; } - done: - VIR_FREE(in); - VIR_FREE(out); - return ret; + return 0; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free temporary variables and change 'cleanup' label to 'error'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 37 +++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 25f0c45955..ad789c611f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3147,9 +3147,8 @@ virSecuritySELinuxSetDaemonSocketLabel(virSecurityManager *mgr G_GNUC_UNUSED, { /* TODO: verify DOI */ virSecurityLabelDef *secdef; - char *scon = NULL; - char *str = NULL; - int rc = -1; + g_autofree char *scon = NULL; + g_autofree char *str = NULL; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (!secdef || !secdef->label) @@ -3159,34 +3158,33 @@ virSecuritySELinuxSetDaemonSocketLabel(virSecurityManager *mgr G_GNUC_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, _("security label driver mismatch: '%1$s' model configured for domain, but hypervisor driver is '%2$s'."), secdef->model, SECURITY_SELINUX_NAME); - goto done; + goto error; } if (getcon_raw(&scon) == -1) { virReportSystemError(errno, _("unable to get current process context '%1$s'"), secdef->label); - goto done; + goto error; } if (!(str = virSecuritySELinuxContextAddRange(secdef->label, scon))) - goto done; + goto error; VIR_DEBUG("Setting VM %s socket context %s", def->name, str); if (setsockcreatecon_raw(str) == -1) { virReportSystemError(errno, _("unable to set socket security context '%1$s'"), str); - goto done; + goto error; } - rc = 0; - done: + return 0; + error: if (security_getenforce() != 1) - rc = 0; - freecon(scon); - VIR_FREE(str); - return rc; + return 0; + + return -1; } static int @@ -3194,7 +3192,6 @@ virSecuritySELinuxSetSocketLabel(virSecurityManager *mgr G_GNUC_UNUSED, virDomainDef *vm) { virSecurityLabelDef *secdef; - int rc = -1; secdef = virDomainDefGetSecurityLabelDef(vm, SECURITY_SELINUX_NAME); if (!secdef || !secdef->label) @@ -3204,7 +3201,7 @@ virSecuritySELinuxSetSocketLabel(virSecurityManager *mgr G_GNUC_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, _("security label driver mismatch: '%1$s' model configured for domain, but hypervisor driver is '%2$s'."), secdef->model, SECURITY_SELINUX_NAME); - goto done; + goto error; } VIR_DEBUG("Setting VM %s socket context %s", @@ -3213,16 +3210,16 @@ virSecuritySELinuxSetSocketLabel(virSecurityManager *mgr G_GNUC_UNUSED, virReportSystemError(errno, _("unable to set socket security context '%1$s'"), secdef->label); - goto done; + goto error; } - rc = 0; + return 0; - done: + error: if (security_getenforce() != 1) - rc = 0; + return 0; - return rc; + return -1; } static int -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free temporary variables and remove the need for 'cleanup' section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 35 ++++++++++++++------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ad789c611f..f01a707c28 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3459,10 +3459,11 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManager *mgr, int fd) { struct stat buf; - char *fcon = NULL; + g_autofree char *fcon = NULL; virSecurityLabelDef *secdef; - char *str = NULL, *proc = NULL, *fd_path = NULL; - int rc = -1; + g_autofree char *str = NULL; + g_autofree char *proc = NULL; + g_autofree char *fd_path = NULL; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (!secdef || !secdef->label) @@ -3470,13 +3471,13 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManager *mgr, if (fstat(fd, &buf) < 0) { virReportSystemError(errno, _("cannot stat tap fd %1$d"), fd); - goto cleanup; + return -1; } if ((buf.st_mode & S_IFMT) != S_IFCHR) { virReportError(VIR_ERR_INTERNAL_ERROR, _("tap fd %1$d is not character device"), fd); - goto cleanup; + return -1; } /* Label /dev/tap([0-9]+)? devices only. Leave /dev/net/tun alone! */ @@ -3485,34 +3486,28 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManager *mgr, if (virFileResolveLink(proc, &fd_path) < 0) { virReportSystemError(errno, _("Unable to resolve link: %1$s"), proc); - goto cleanup; + return -1; } if (!STRPREFIX(fd_path, "/dev/tap")) { VIR_DEBUG("fd=%d points to %s not setting SELinux label", fd, fd_path); - rc = 0; - goto cleanup; + return 0; } if (getContext(mgr, fd_path, buf.st_mode, &fcon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot lookup default selinux label for tap fd %1$d"), fd); - goto cleanup; + return -1; } - if (!(str = virSecuritySELinuxContextAddRange(secdef->label, fcon))) { - goto cleanup; - } else { - rc = virSecuritySELinuxFSetFilecon(fd, str); - } + if (!(str = virSecuritySELinuxContextAddRange(secdef->label, fcon))) + return -1; - cleanup: - freecon(fcon); - VIR_FREE(fd_path); - VIR_FREE(proc); - VIR_FREE(str); - return rc; + if (virSecuritySELinuxFSetFilecon(fd, str) < 0) + return -1; + + return 0; } static char * -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> The selinux headers have a 'typedef context_s_t *context_t;' definition in the header so we declare autoptr cleanup function for 'context_s_t' and use it instead of 'context_t' definitions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f01a707c28..87348f36fa 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -48,6 +48,9 @@ VIR_LOG_INIT("security.security_selinux"); #define MAX_CONTEXT 1024 +/* selinux headers define 'context_t' as pointer to 'context_s_t' */ +G_DEFINE_AUTOPTR_CLEANUP_FUNC(context_s_t, context_free); + typedef struct _virSecuritySELinuxData virSecuritySELinuxData; struct _virSecuritySELinuxData { char *domain_context; -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Use the new autoptr helper to free the temporary variables and refactor cleanup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 87348f36fa..55a5593fa5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -572,37 +572,31 @@ virSecuritySELinuxContextAddRange(const char *src, const char *dst) { const char *str = NULL; - char *ret = NULL; - context_t srccon = NULL; - context_t dstcon = NULL; + g_autoptr(context_s_t) srccon = NULL; + g_autoptr(context_s_t) dstcon = NULL; if (!src || !dst) - return ret; + return NULL; if (!(srccon = context_new(src)) || !(dstcon = context_new(dst))) { virReportSystemError(errno, "%s", _("unable to allocate security context")); - goto cleanup; + return NULL; } if (context_range_set(dstcon, context_range_get(srccon)) == -1) { virReportSystemError(errno, _("unable to set security context range '%1$s'"), dst); - goto cleanup; + return NULL; } if (!(str = context_str(dstcon))) { virReportSystemError(errno, "%s", _("Unable to format SELinux context")); - goto cleanup; + return NULL; } - ret = g_strdup(str); - - cleanup: - if (srccon) context_free(srccon); - if (dstcon) context_free(dstcon); - return ret; + return g_strdup(str); } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Use automatic freeing of temporary variables and remove cleanup section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 55a5593fa5..67d9da461a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -650,11 +650,10 @@ virSecuritySELinuxGenNewContext(const char *basecontext, const char *mcs, bool isObjectContext) { - context_t context = NULL; - char *ret = NULL; + g_autoptr(context_s_t) context = NULL; const char *str; - char *ourSecContext = NULL; - context_t ourContext = NULL; + g_autofree char *ourSecContext = NULL; + g_autoptr(context_s_t) ourContext = NULL; VIR_DEBUG("basecontext=%s mcs=%s isObjectContext=%d", basecontext, mcs, isObjectContext); @@ -662,13 +661,13 @@ virSecuritySELinuxGenNewContext(const char *basecontext, if (getcon_raw(&ourSecContext) < 0) { virReportSystemError(errno, "%s", _("Unable to get current process SELinux context")); - goto cleanup; + return NULL; } if (!(ourContext = context_new(ourSecContext))) { virReportSystemError(errno, _("Unable to parse current SELinux context '%1$s'"), ourSecContext); - goto cleanup; + return NULL; } VIR_DEBUG("process=%s", ourSecContext); @@ -676,7 +675,7 @@ virSecuritySELinuxGenNewContext(const char *basecontext, virReportSystemError(errno, _("Unable to parse base SELinux context '%1$s'"), basecontext); - goto cleanup; + return NULL; } if (context_user_set(context, @@ -684,7 +683,7 @@ virSecuritySELinuxGenNewContext(const char *basecontext, virReportSystemError(errno, _("Unable to set SELinux context user '%1$s'"), context_user_get(ourContext)); - goto cleanup; + return NULL; } if (!isObjectContext && @@ -693,27 +692,23 @@ virSecuritySELinuxGenNewContext(const char *basecontext, virReportSystemError(errno, _("Unable to set SELinux context role '%1$s'"), context_role_get(ourContext)); - goto cleanup; + return NULL; } if (context_range_set(context, mcs) != 0) { virReportSystemError(errno, _("Unable to set SELinux context MCS '%1$s'"), mcs); - goto cleanup; + return NULL; } if (!(str = context_str(context))) { virReportSystemError(errno, "%s", _("Unable to format SELinux context")); - goto cleanup; + return NULL; } - ret = g_strdup(str); - VIR_DEBUG("Generated context '%s'", ret); - cleanup: - freecon(ourSecContext); - context_free(ourContext); - context_free(context); - return ret; + + VIR_DEBUG("Generated context '%s'", str); + return g_strdup(str); } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free temporary variables in order to remove 'cleanup' section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 67d9da461a..3004320380 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1025,8 +1025,8 @@ virSecuritySELinuxReserveLabel(virSecurityManager *mgr, virDomainDef *def, pid_t pid) { - char *pctx; - context_t ctx = NULL; + g_autofree char *pctx = NULL; + g_autoptr(context_s_t) ctx = NULL; const char *mcs; int rv; virSecurityLabelDef *seclabel; @@ -1045,31 +1045,23 @@ virSecuritySELinuxReserveLabel(virSecurityManager *mgr, ctx = context_new(pctx); if (!ctx) - goto error; + return -1; mcs = context_range_get(ctx); if (!mcs) - goto error; + return -1; if ((rv = virSecuritySELinuxMCSAdd(mgr, mcs)) < 0) - goto error; + return -1; if (rv == 1) { virReportError(VIR_ERR_INTERNAL_ERROR, _("MCS level for existing domain label %1$s already reserved"), (char*)pctx); - goto error; + return -1; } - freecon(pctx); - context_free(ctx); - return 0; - - error: - freecon(pctx); - context_free(ctx); - return -1; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3004320380..54f12d044f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1271,7 +1271,7 @@ virSecuritySELinuxGetProcessLabel(virSecurityManager *mgr G_GNUC_UNUSED, pid_t pid, virSecurityLabelPtr sec) { - char *ctx; + g_autofree char *ctx = NULL; if (getpidcon_raw(pid, &ctx) == -1) { virReportSystemError(errno, @@ -1284,12 +1284,9 @@ virSecuritySELinuxGetProcessLabel(virSecurityManager *mgr G_GNUC_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, _("security label exceeds maximum length: %1$d"), VIR_SECURITY_LABEL_BUFLEN - 1); - freecon(ctx); return -1; } - freecon(ctx); - VIR_DEBUG("label=%s", sec->label); sec->enforcing = security_getenforce(); if (sec->enforcing == -1) { -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 54f12d044f..b390e0c95f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1380,7 +1380,7 @@ virSecuritySELinuxSetFilecon(virSecurityManager *mgr, bool remember) { bool privileged = virSecurityManagerGetPrivileged(mgr); - char *econ = NULL; + g_autofree char *econ = NULL; int refcount; int rc; bool rollback = false; @@ -1454,7 +1454,6 @@ virSecuritySELinuxSetFilecon(virSecurityManager *mgr, virErrorRestore(&origerr); } - freecon(econ); return ret; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Skip the use of temporary variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b390e0c95f..b16ab14dfe 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1602,7 +1602,6 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManager *mgr, virDomainDef *def, virDomainInputDef *input) { - int rc = 0; virSecurityLabelDef *seclabel; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); @@ -1612,8 +1611,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManager *mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: case VIR_DOMAIN_INPUT_TYPE_EVDEV: - rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, true); - break; + return virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, true); case VIR_DOMAIN_INPUT_TYPE_MOUSE: case VIR_DOMAIN_INPUT_TYPE_TABLET: @@ -1622,7 +1620,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManager *mgr, break; } - return rc; + return 0; } -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Automatically free temporary variables in order to remove 'cleanup' section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b16ab14dfe..9d14e33340 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3495,34 +3495,29 @@ virSecuritySELinuxGenImageLabel(virSecurityManager *mgr, virSecurityLabelDef *secdef; virSecuritySELinuxData *data = virSecurityManagerGetPrivateData(mgr); const char *range; - context_t ctx = NULL; - char *label = NULL; - char *mcs = NULL; + g_autoptr(context_s_t) ctx = NULL; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) - goto cleanup; + return NULL; if (secdef->label) { ctx = context_new(secdef->label); if (!ctx) { virReportSystemError(errno, _("unable to create selinux context for: %1$s"), secdef->label); - goto cleanup; + return NULL; } range = context_range_get(ctx); if (range) { - mcs = g_strdup(range); - if (!(label = virSecuritySELinuxGenNewContext(data->file_context, - mcs, true))) - goto cleanup; + g_autofree char *mcs = g_strdup(range); + + return virSecuritySELinuxGenNewContext(data->file_context, + mcs, true); } } - cleanup: - context_free(ctx); - VIR_FREE(mcs); - return label; + return NULL; } static char * -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Fix linebreaks and remove the use of ternary operator. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/security/security_selinux.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9d14e33340..fa5d1568eb 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3526,23 +3526,24 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManager *mgr, { char *opts = NULL; virSecurityLabelDef *secdef; + const char *imagelabel = NULL; if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) { if (!secdef->imagelabel) secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr, def); if (secdef->imagelabel) { - opts = g_strdup_printf( - ",context=\"%s\"", + opts = g_strdup_printf(",context=\"%s\"", (const char*) secdef->imagelabel); } + + imagelabel = secdef->imagelabel; } if (!opts) opts = g_strdup(""); - VIR_DEBUG("imageLabel=%s opts=%s", - secdef ? secdef->imagelabel : "(null)", opts); + VIR_DEBUG("imageLabel=%s opts=%s", NULLSTR(imagelabel), opts); return opts; } -- 2.50.0
On a Monday in 2025, Peter Krempa via Devel wrote:
Some cleanups resutling from analyzing some selinux code.
Peter Krempa (19): security/security_driver.c/h: Fix function header formatting virSecuritySELinuxTransactionRun: Refactor cleanup virSecuritySELinuxMCSFind: Refactor variable clearing virSecuritySELinuxRestoreFileLabel: Refactor cleanup virSecuritySELinux(Set|Restore)TPMFileLabel: Automatically free 'cancel_path' virSecuritySELinuxRestoreFileLabels: Refactor variable freeing virSecuritySELinux(Set|Restore)HostdevCapsLabel: Refactor cleanup virSecuritySELinux(Set|Restore)ChardevLabel: Refactor cleanup virSecuritySELinuxSet(Daemon)SocketLabel: Refactor cleanup virSecuritySELinuxSetTapFDLabel: Refactor cleanup security_selinux: Declare internal autoptr cleanup helper for 'context_t' virSecuritySELinuxContextAddRange: Refactor cleanup of 'context_t' virSecuritySELinuxGenNewContext: Refactor cleanup virSecuritySELinuxReserveLabel: Refactor cleanup virSecuritySELinuxGetProcessLabel: Automatically free 'ctx' temp variable virSecuritySELinuxSetFilecon: Automatically free 'econ' temp variable virSecuritySELinuxRestoreInputLabel: Return values directly virSecuritySELinuxGenImageLabel: Refactor cleanup virSecuritySELinuxGetSecurityMountOptions: refactor printing
src/security/security_driver.c | 5 +- src/security/security_driver.h | 5 +- src/security/security_selinux.c | 347 +++++++++++++------------------- 3 files changed, 149 insertions(+), 208 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa