[libvirt] [PATCH v3 0/2] selinux: Only create the selabel_handle once.
Version 3 of this patch. Because I now have to pass the 'mgr' pointer around, the patch is considerably more complicated than before. Patch 1/2 is required so that I can use virReportSystemError when I don't need to have any optional arguments, ie. the equivalent of: printf ("foo\n"); I have tested these two patches against the libguestfs test suite, and it works for me and removes the memory leak. Rich.
From: "Richard W.M. Jones" <rjones@redhat.com> --- src/util/virerror.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virerror.h b/src/util/virerror.h index 332a5eb..ac6d746 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -62,11 +62,11 @@ void virReportSystemErrorFull(int domcode, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(6, 7); -# define virReportSystemError(theerrno, fmt,...) \ +# define virReportSystemError(theerrno, ...) \ virReportSystemErrorFull(VIR_FROM_THIS, \ (theerrno), \ __FILE__, __FUNCTION__, __LINE__, \ - (fmt), __VA_ARGS__) + __VA_ARGS__) # define virReportInvalidNullArg(argname) \ virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ -- 1.8.1
From: "Richard W.M. Jones" <rjones@redhat.com> According to Eric Paris this is slightly more efficient because it only loads the regular expressions in libselinux once. --- src/security/security_selinux.c | 129 ++++++++++++++++++++++++++-------------- 1 file changed, 83 insertions(+), 46 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a3ef728..d1f80b2 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -63,6 +63,9 @@ struct _virSecuritySELinuxData { char *content_context; virHashTablePtr mcs; bool skipAllLabel; +#if HAVE_SELINUX_LABEL_H + struct selabel_handle *label_handle; +#endif }; struct _virSecuritySELinuxCallbackData { @@ -367,12 +370,21 @@ virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr) data->skipAllLabel = true; +#if HAVE_SELINUX_LABEL_H + data->label_handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); + if (!data->label_handle) { + virReportSystemError(errno, + _("cannot open SELinux label_handle")); + return -1; + } +#endif + selinux_conf = virConfReadFile(selinux_lxc_contexts_path(), 0); if (!selinux_conf) { virReportSystemError(errno, _("cannot open SELinux lxc contexts file '%s'"), selinux_lxc_contexts_path()); - return -1; + goto error; } scon = virConfGetValue(selinux_conf, "process"); @@ -418,6 +430,9 @@ virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr) return 0; error: +#if HAVE_SELINUX_LABEL_H + selabel_close(data->label_handle); +#endif virConfFree(selinux_conf); VIR_FREE(data->domain_context); VIR_FREE(data->file_context); @@ -444,6 +459,15 @@ virSecuritySELinuxQEMUInitialize(virSecurityManagerPtr mgr) data->skipAllLabel = false; +#if HAVE_SELINUX_LABEL_H + data->label_handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); + if (!data->label_handle) { + virReportSystemError(errno, + _("cannot open SELinux label_handle")); + return -1; + } +#endif + if (virFileReadAll(selinux_virtual_domain_context_path(), MAX_CONTEXT, &(data->domain_context)) < 0) { virReportSystemError(errno, _("cannot read SELinux virtual domain context file '%s'"), @@ -499,6 +523,9 @@ virSecuritySELinuxQEMUInitialize(virSecurityManagerPtr mgr) return 0; error: +#if HAVE_SELINUX_LABEL_H + selabel_close(data->label_handle); +#endif VIR_FREE(data->domain_context); VIR_FREE(data->alt_domain_context); VIR_FREE(data->file_context); @@ -763,6 +790,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) if (!data) return 0; +#if HAVE_SELINUX_LABEL_H + selabel_close(data->label_handle); +#endif + virHashFree(data->mcs); VIR_FREE(data->domain_context); @@ -937,18 +968,13 @@ virSecuritySELinuxFSetFilecon(int fd, char *tcon) /* Set fcon to the appropriate label for path and mode, or return -1. */ static int -getContext(const char *newpath, mode_t mode, security_context_t *fcon) +getContext(virSecurityManagerPtr mgr, + const char *newpath, mode_t mode, security_context_t *fcon) { #if HAVE_SELINUX_LABEL_H - struct selabel_handle *handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); - int ret; - - if (handle == NULL) - return -1; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); - ret = selabel_lookup_raw(handle, fcon, newpath, mode); - selabel_close(handle); - return ret; + return selabel_lookup_raw(data->label_handle, fcon, newpath, mode); #else return matchpathcon(newpath, mode, fcon); #endif @@ -958,7 +984,8 @@ getContext(const char *newpath, mode_t mode, security_context_t *fcon) /* This method shouldn't raise errors, since they'll overwrite * errors that the caller(s) are already dealing with */ static int -virSecuritySELinuxRestoreSecurityFileLabel(const char *path) +virSecuritySELinuxRestoreSecurityFileLabel(virSecurityManagerPtr mgr, + const char *path) { struct stat buf; security_context_t fcon = NULL; @@ -980,7 +1007,7 @@ virSecuritySELinuxRestoreSecurityFileLabel(const char *path) goto err; } - if (getContext(newpath, buf.st_mode, &fcon) < 0) { + if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) { /* Any user created path likely does not have a default label, * which makes this an expected non error */ @@ -997,7 +1024,7 @@ err: } static int -virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk, int migrated) @@ -1044,7 +1071,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr ATTRIBU } } - return virSecuritySELinuxRestoreSecurityFileLabel(disk->src); + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, disk->src); } @@ -1301,22 +1328,27 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN static int virSecuritySELinuxRestoreSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecuritySELinuxRestoreSecurityFileLabel(file); + virSecurityManagerPtr mgr = opaque; + + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, file); } static int virSecuritySELinuxRestoreSecurityUSBLabel(usbDevice *dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecuritySELinuxRestoreSecurityFileLabel(file); + virSecurityManagerPtr mgr = opaque; + + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, file); } static int -virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virDomainHostdevDefPtr dev, +virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, + virDomainHostdevDefPtr dev, const char *vroot) { @@ -1335,7 +1367,7 @@ virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virDomainHostdevDefPtr dev, if (!usb) goto done; - ret = usbDeviceFileIterate(usb, virSecuritySELinuxRestoreSecurityUSBLabel, NULL); + ret = usbDeviceFileIterate(usb, virSecuritySELinuxRestoreSecurityUSBLabel, mgr); usbFreeDevice(usb); break; @@ -1350,7 +1382,7 @@ virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virDomainHostdevDefPtr dev, if (!pci) goto done; - ret = pciDeviceFileIterate(pci, virSecuritySELinuxRestoreSecurityPCILabel, NULL); + ret = pciDeviceFileIterate(pci, virSecuritySELinuxRestoreSecurityPCILabel, mgr); pciFreeDevice(pci); break; @@ -1367,7 +1399,8 @@ done: static int -virSecuritySELinuxRestoreSecurityHostdevCapsLabel(virDomainHostdevDefPtr dev, +virSecuritySELinuxRestoreSecurityHostdevCapsLabel(virSecurityManagerPtr mgr, + virDomainHostdevDefPtr dev, const char *vroot) { int ret = -1; @@ -1387,7 +1420,7 @@ virSecuritySELinuxRestoreSecurityHostdevCapsLabel(virDomainHostdevDefPtr dev, return -1; } } - ret = virSecuritySELinuxRestoreSecurityFileLabel(path); + ret = virSecuritySELinuxRestoreSecurityFileLabel(mgr, path); VIR_FREE(path); break; } @@ -1405,7 +1438,7 @@ virSecuritySELinuxRestoreSecurityHostdevCapsLabel(virDomainHostdevDefPtr dev, return -1; } } - ret = virSecuritySELinuxRestoreSecurityFileLabel(path); + ret = virSecuritySELinuxRestoreSecurityFileLabel(mgr, path); VIR_FREE(path); break; } @@ -1420,7 +1453,7 @@ virSecuritySELinuxRestoreSecurityHostdevCapsLabel(virDomainHostdevDefPtr dev, static int -virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot) @@ -1437,10 +1470,10 @@ virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUT switch (dev->mode) { case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: - return virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(dev, vroot); + return virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(mgr, dev, vroot); case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: - return virSecuritySELinuxRestoreSecurityHostdevCapsLabel(dev, vroot); + return virSecuritySELinuxRestoreSecurityHostdevCapsLabel(mgr, dev, vroot); default: return 0; @@ -1522,7 +1555,8 @@ done: } static int -virSecuritySELinuxRestoreSecurityChardevLabel(virDomainDefPtr def, +virSecuritySELinuxRestoreSecurityChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, virDomainChrDefPtr dev, virDomainChrSourceDefPtr dev_source) @@ -1545,14 +1579,14 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virDomainDefPtr def, switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - if (virSecuritySELinuxRestoreSecurityFileLabel(dev_source->data.file.path) < 0) + if (virSecuritySELinuxRestoreSecurityFileLabel(mgr, dev_source->data.file.path) < 0) goto done; ret = 0; break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { - if (virSecuritySELinuxRestoreSecurityFileLabel(dev_source->data.file.path) < 0) + if (virSecuritySELinuxRestoreSecurityFileLabel(mgr, dev_source->data.file.path) < 0) goto done; } ret = 0; @@ -1565,11 +1599,11 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virDomainDefPtr def, goto done; } if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxRestoreSecurityFileLabel(out) < 0) || - (virSecuritySELinuxRestoreSecurityFileLabel(in) < 0)) { + if ((virSecuritySELinuxRestoreSecurityFileLabel(mgr, out) < 0) || + (virSecuritySELinuxRestoreSecurityFileLabel(mgr, in) < 0)) { goto done; } - } else if (virSecuritySELinuxRestoreSecurityFileLabel(dev_source->data.file.path) < 0) { + } else if (virSecuritySELinuxRestoreSecurityFileLabel(mgr, dev_source->data.file.path) < 0) { goto done; } ret = 0; @@ -1590,14 +1624,16 @@ done: static int virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def, virDomainChrDefPtr dev, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { + virSecurityManagerPtr mgr = opaque; + /* This is taken care of by processing of def->serials */ if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) return 0; - return virSecuritySELinuxRestoreSecurityChardevLabel(def, dev, + return virSecuritySELinuxRestoreSecurityChardevLabel(mgr, def, dev, &dev->source); } @@ -1605,8 +1641,9 @@ virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def, static int virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, virDomainSmartcardDefPtr dev, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { + virSecurityManagerPtr mgr = opaque; const char *database; switch (dev->type) { @@ -1617,10 +1654,10 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxRestoreSecurityFileLabel(database); + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, database); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: - return virSecuritySELinuxRestoreSecurityChardevLabel(def, NULL, &dev->data.passthru); + return virSecuritySELinuxRestoreSecurityChardevLabel(mgr, def, NULL, &dev->data.passthru); default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1670,21 +1707,21 @@ virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, if (virDomainChrDefForeach(def, false, virSecuritySELinuxRestoreSecurityChardevCallback, - NULL) < 0) + mgr) < 0) rc = -1; if (virDomainSmartcardDefForeach(def, false, virSecuritySELinuxRestoreSecuritySmartcardCallback, - NULL) < 0) + mgr) < 0) rc = -1; if (def->os.kernel && - virSecuritySELinuxRestoreSecurityFileLabel(def->os.kernel) < 0) + virSecuritySELinuxRestoreSecurityFileLabel(mgr, def->os.kernel) < 0) rc = -1; if (def->os.initrd && - virSecuritySELinuxRestoreSecurityFileLabel(def->os.initrd) < 0) + virSecuritySELinuxRestoreSecurityFileLabel(mgr, def->os.initrd) < 0) rc = -1; return rc; @@ -1737,7 +1774,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, static int -virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile) { @@ -1750,7 +1787,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNU if (secdef->norelabel) return 0; - return virSecuritySELinuxRestoreSecurityFileLabel(savefile); + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, savefile); } @@ -2080,7 +2117,7 @@ virSecuritySELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, } static int -virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, int fd) { @@ -2108,7 +2145,7 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if (getContext("/dev/tap.*", buf.st_mode, &fcon) < 0) { + if (getContext(mgr, "/dev/tap.*", buf.st_mode, &fcon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot lookup default selinux label for tap fd %d"), fd); goto cleanup; -- 1.8.1
On Thu, Jan 24, 2013 at 10:10:58AM +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones@redhat.com>
According to Eric Paris this is slightly more efficient because it only loads the regular expressions in libselinux once. --- src/security/security_selinux.c | 129 ++++++++++++++++++++++++++-------------- 1 file changed, 83 insertions(+), 46 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a3ef728..d1f80b2 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -63,6 +63,9 @@ struct _virSecuritySELinuxData { char *content_context; virHashTablePtr mcs; bool skipAllLabel; +#if HAVE_SELINUX_LABEL_H + struct selabel_handle *label_handle; +#endif };
struct _virSecuritySELinuxCallbackData { @@ -367,12 +370,21 @@ virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr)
data->skipAllLabel = true;
+#if HAVE_SELINUX_LABEL_H + data->label_handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); + if (!data->label_handle) { + virReportSystemError(errno, + _("cannot open SELinux label_handle"));
This is missing "%s",
+ return -1; + } +#endif + selinux_conf = virConfReadFile(selinux_lxc_contexts_path(), 0); if (!selinux_conf) { virReportSystemError(errno, _("cannot open SELinux lxc contexts file '%s'"), selinux_lxc_contexts_path()); - return -1; + goto error; }
scon = virConfGetValue(selinux_conf, "process"); @@ -418,6 +430,9 @@ virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr) return 0;
error: +#if HAVE_SELINUX_LABEL_H + selabel_close(data->label_handle); +#endif virConfFree(selinux_conf); VIR_FREE(data->domain_context); VIR_FREE(data->file_context); @@ -444,6 +459,15 @@ virSecuritySELinuxQEMUInitialize(virSecurityManagerPtr mgr)
data->skipAllLabel = false;
+#if HAVE_SELINUX_LABEL_H + data->label_handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); + if (!data->label_handle) { + virReportSystemError(errno, + _("cannot open SELinux label_handle"));
This is missing "%s", Rest of the patch looks fine though. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Jan 24, 2013 at 10:10:56AM +0000, Richard W.M. Jones wrote:
Version 3 of this patch. Because I now have to pass the 'mgr' pointer around, the patch is considerably more complicated than before.
Patch 1/2 is required so that I can use virReportSystemError when I don't need to have any optional arguments, ie. the equivalent of:
printf ("foo\n");
No, that's not allowed - everything must have a format string - even static messages - it should instead be: printf ("%s", _("foo\n")); make syntax-check ought to have complained about this IIRC. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Jan 24, 2013 at 10:17:16AM +0000, Daniel P. Berrange wrote:
On Thu, Jan 24, 2013 at 10:10:56AM +0000, Richard W.M. Jones wrote:
Version 3 of this patch. Because I now have to pass the 'mgr' pointer around, the patch is considerably more complicated than before.
Patch 1/2 is required so that I can use virReportSystemError when I don't need to have any optional arguments, ie. the equivalent of:
printf ("foo\n");
No, that's not allowed - everything must have a format string - even static messages - it should instead be:
printf ("%s", _("foo\n"));
make syntax-check ought to have complained about this IIRC.
I don't see why. Presumably the worry is that the translator will introduce a %-sequence into the string? That should be picked up by one of the msg* utilities. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 01/24/2013 03:24 AM, Richard W.M. Jones wrote:
On Thu, Jan 24, 2013 at 10:17:16AM +0000, Daniel P. Berrange wrote:
On Thu, Jan 24, 2013 at 10:10:56AM +0000, Richard W.M. Jones wrote:
Version 3 of this patch. Because I now have to pass the 'mgr' pointer around, the patch is considerably more complicated than before.
Patch 1/2 is required so that I can use virReportSystemError when I don't need to have any optional arguments, ie. the equivalent of:
printf ("foo\n");
No, that's not allowed - everything must have a format string - even static messages - it should instead be:
printf ("%s", _("foo\n"));
make syntax-check ought to have complained about this IIRC.
I don't see why. Presumably the worry is that the translator will introduce a %-sequence into the string? That should be picked up by one of the msg* utilities.
Whether or not it is picked up by the msg* utilities, there is also the issue of whether xgettext will mark the string as being printf-format when extracting the string for translation, as well as the issue that there are existing compilers (at least the older gcc on FreeBSD) that loudly complain (and thus break a -Werror build) if given a function marked __attribute__((__printf__(...))) but with no % in the format string. So just because the standard allows you to use printf with a format string with no % doesn't mean that we like that style. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Richard W.M. Jones