The public API entry points will call virDispatchError which will print to stderr by default. We then jump to a cleanup path which calls virDispatchError again. We tried to stop the entry points printing to stderr, but incorrectly called virSetErrorFunc. It needs a real function that is a no-op, not a NULL function. Once we fix virSetErrorFunc, then we need to use fprintf in the cleanup path instead of virDispatchError. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virt-login-shell.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index b906fa9ed6..8ffc72ab9a 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -143,6 +143,12 @@ show_version(void) } +static void +hideErrorFunc(void *opaque ATTRIBUTE_UNUSED, + virErrorPtr err ATTRIBUTE_UNUSED) +{ +} + int main(int argc, char **argv) { @@ -186,7 +192,7 @@ main(int argc, char **argv) return EXIT_CANCELED; } - virSetErrorFunc(NULL, NULL); + virSetErrorFunc(NULL, hideErrorFunc); virSetErrorLogPriorityFunc(NULL); progname = argv[0]; @@ -403,7 +409,7 @@ main(int argc, char **argv) if (saved_err) { virSetError(saved_err); - virDispatchError(NULL); + fprintf(stderr, "%s: %s\n", argv[0], virGetLastErrorMessage()); } return ret; } -- 2.21.0

The virt-login-shell binary is a setuid program that takes no arguments. When invoked it looks at the invoking uid, resolves it to a username, and finds an LXC guest with the same name. It then starts the guest and runs the shell in side the namespaces of the container. Given this set of tasks the virt-login-shell binary needs to connect to libvirtd, make various other libvirt API calls. This is a problem for setuid binaries as various libraries that libvirt.so links to are not safe. For example, they have constructor functions which execute an unknown amount of code that can be influenced by env variables. For this reason virt-login-shell doesn't use libvirt.so, but instead links to a custom, cut down, set of source files sufficient to be a local client only. This introduces a problem for integrating glib2 into libvirt though, as once integrated, there would be no way to build virt-login-shell without an external dependancy on glib2 and this is definitely not setuid safe. To resolve this problem, we split the virt-login-shell binary into two parts. The first part is setuid and does almost nothing. It simply records the original uid+gid, and then invokes the virt-login-shell-helper binary. Crucially when it does this it completes scrubs all environment variables. It is thus safe for virt-login-shell-helper to link to the normal libvirt.so. Any things that constructor functions do cannot be influenced by user control env vars or cli args. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .gitignore | 1 + cfg.mk | 10 ++-- libvirt.spec.in | 1 + tools/Makefile.am | 23 ++++----- tools/virt-login-shell-helper.c | 38 ++++++++++++--- tools/virt-login-shell.c | 84 +++++++++++++++++++++++++++++++++ 6 files changed, 134 insertions(+), 23 deletions(-) create mode 100644 tools/virt-login-shell.c diff --git a/.gitignore b/.gitignore index 727bfdb6ec..f3193173d6 100644 --- a/.gitignore +++ b/.gitignore @@ -183,6 +183,7 @@ /tests/test_conf /tools/libvirt-guests.sh /tools/virt-login-shell +/tools/virt-login-shell-helper /tools/virsh /tools/virsh-*-edit.c /tools/virt-admin diff --git a/cfg.mk b/cfg.mk index 694ce2076f..9130b4560b 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1203,7 +1203,7 @@ exclude_file_name_regexp--sc_avoid_write = \ exclude_file_name_regexp--sc_bindtextdomain = .* -exclude_file_name_regexp--sc_gettext_init = ^(tests|examples)/ +exclude_file_name_regexp--sc_gettext_init = ^((tests|examples)/|tools/virt-login-shell.c) exclude_file_name_regexp--sc_copyright_format = \ ^cfg\.mk$$ @@ -1229,7 +1229,7 @@ exclude_file_name_regexp--sc_prohibit_access_xok = \ ^(cfg\.mk|src/util/virutil\.c)$$ exclude_file_name_regexp--sc_prohibit_asprintf = \ - ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) + ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c|tools/virt-login-shell\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$) @@ -1256,7 +1256,7 @@ exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ exclude_file_name_regexp--sc_prohibit_nonreentrant = \ - ^((po|tests|examples)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl$$) + ^((po|tests|examples)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl|tools/virt-login-shell\.c$$) exclude_file_name_regexp--sc_prohibit_select = \ ^cfg\.mk$$ @@ -1270,7 +1270,7 @@ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ -exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c$$ +exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c|tools/virt-login-shell\.c$$ exclude_file_name_regexp--sc_prohibit_sprintf = \ ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$ @@ -1317,7 +1317,7 @@ exclude_file_name_regexp--sc_prohibit_unsigned_pid = \ ^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$ exclude_file_name_regexp--sc_prohibit_getenv = \ - ^tests/.*\.[ch]$$ + ^tests/.*\.[ch]|tools/virt-login-shell\.c$$ exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = \ ^(src/util/virlog\.h|src/network/bridge_driver\.h)$$ diff --git a/libvirt.spec.in b/libvirt.spec.in index 045c0fed1a..6f96fbec33 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1847,6 +1847,7 @@ exit 0 %if %{with_lxc} %files login-shell %attr(4750, root, virtlogin) %{_bindir}/virt-login-shell +%{_libexecdir}/virt-login-shell-helper %config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf %{_mandir}/man1/virt-login-shell.1* %endif diff --git a/tools/Makefile.am b/tools/Makefile.am index 125540d313..3d9461ba65 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -105,6 +105,7 @@ endif WITH_SANLOCK if WITH_LOGIN_SHELL conf_DATA += virt-login-shell.conf bin_PROGRAMS += virt-login-shell +libexec_PROGRAMS = virt-login-shell-helper man1_MANS += virt-login-shell.1 endif WITH_LOGIN_SHELL @@ -192,25 +193,25 @@ virt_host_validate_CFLAGS = \ $(AM_CFLAGS) \ $(NULL) -# Since virt-login-shell will be setuid, we must do everything -# we can to avoid linking to other libraries. Many of them do -# unsafe things in functions marked __atttribute__((constructor)). -# This we statically link to a library containing only the minimal -# libvirt client code, not libvirt.so itself. +# virt-login-shell will be setuid, and must not link to anything +# except glibc. It wil scrub the environment and then invoke the +# real virt-login-shell-helper binary. virt_login_shell_SOURCES = \ + virt-login-shell.c + +virt_login_shell_helper_SOURCES = \ virt-login-shell-helper.c -virt_login_shell_LDFLAGS = \ +virt_login_shell_helper_LDFLAGS = \ $(AM_LDFLAGS) \ $(PIE_LDFLAGS) \ $(NULL) -virt_login_shell_LDADD = \ - $(STATIC_BINARIES) \ - ../src/libvirt-setuid-rpc-client.la \ +virt_login_shell_helper_LDADD = \ + ../src/libvirt.la \ + ../src/libvirt-lxc.la \ ../gnulib/lib/libgnu.la -virt_login_shell_CFLAGS = \ - -DLIBVIRT_SETUID_RPC_CLIENT \ +virt_login_shell_helper_CFLAGS = \ $(AM_CFLAGS) \ $(NULL) diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c index f06eb1464a..8dc3bedaa0 100644 --- a/tools/virt-login-shell-helper.c +++ b/tools/virt-login-shell-helper.c @@ -157,8 +157,10 @@ main(int argc, char **argv) pid_t cpid = -1; int ret = EXIT_CANCELED; int status; - uid_t uid = getuid(); - gid_t gid = getgid(); + unsigned long long uidval; + unsigned long long gidval; + uid_t uid; + gid_t gid; char *name = NULL; char **shargv = NULL; size_t shargvlen = 0; @@ -199,6 +201,16 @@ main(int argc, char **argv) if (virGettextInitialize() < 0) return ret; + if (geteuid() != 0) { + fprintf(stderr, _("%s: must be run as root\n"), argv[0]); + return ret; + } + + if (getuid() != 0) { + fprintf(stderr, _("%s: must not be run setuid root\n"), argv[0]); + return ret; + } + while ((arg = getopt_long(argc, argv, "hVc:", opt, &longindex)) != -1) { switch (arg) { case 'h': @@ -220,17 +232,29 @@ main(int argc, char **argv) } } - if (argc > optind) { - virReportSystemError(EINVAL, _("%s takes no options"), progname); + if (optind != (argc - 2)) { + virReportSystemError(EINVAL, _("%s expects UID and GID parameters"), progname); goto cleanup; } - if (uid == 0) { - virReportSystemError(EPERM, _("%s must be run by non root users"), - progname); + if (virStrToLong_ull(argv[optind], NULL, 10, &uidval) < 0 || + ((uid_t)uidval) != uidval) { + virReportSystemError(EINVAL, _("%s cannot parse UID '%s'"), + progname, argv[optind]); goto cleanup; } + optind++; + if (virStrToLong_ull(argv[optind], NULL, 10, &gidval) < 0 || + ((gid_t)gidval) != gidval) { + virReportSystemError(EINVAL, _("%s cannot parse GID '%s'"), + progname, argv[optind]); + goto cleanup; + } + + uid = (uid_t)uidval; + gid = (gid_t)gidval; + name = virGetUserName(uid); if (!name) goto cleanup; diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c new file mode 100644 index 0000000000..41d0b349aa --- /dev/null +++ b/tools/virt-login-shell.c @@ -0,0 +1,84 @@ +/* + * virt-login-shell.c: a setuid shell to connect to a container + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include <unistd.h> +#include <sys/types.h> +#include <assert.h> +#include <stdio.h> +#include <stdlib.h> +#include <errno.h> +#include <string.h> + +#include "configmake.h" +#include "intprops.h" + +int main(int argc, char **argv) { + char uidstr[INT_BUFSIZE_BOUND(uid_t)]; + char gidstr[INT_BUFSIZE_BOUND(gid_t)]; + const char *const newargv[] = { + LIBEXECDIR "/virt-login-shell-helper", + uidstr, + gidstr, + NULL, + }; + char *newenv[] = { + NULL, + NULL, + }; + char *term = getenv("TERM"); + + if (getuid() == 0 || getgid() == 0) { + fprintf(stderr, "%s: must not be run as root\n", argv[0]); + exit(EXIT_FAILURE); + } + + if (geteuid() != 0) { + fprintf(stderr, "%s: must be run as setuid root\n", argv[0]); + exit(EXIT_FAILURE); + } + + if (argc != 1) { + fprintf(stderr, "%s: no arguments expected\n", argv[0]); + exit(EXIT_FAILURE); + } + + if (term && + asprintf(&(newenv[0]), "TERM=%s", term) < 0) { + fprintf(stderr, "%s: cannot set TERM env variable: %s\n", + argv[0], strerror(errno)); + exit(EXIT_FAILURE); + } + + assert(snprintf(uidstr, sizeof(uidstr), "%d", getuid()) < sizeof(uidstr)); + assert(snprintf(gidstr, sizeof(gidstr), "%d", getgid()) < sizeof(gidstr)); + + if (setuid(0) < 0) { + fprintf(stderr, "%s: unable to set real UID to root: %s\n", + argv[0], strerror(errno)); + exit(EXIT_FAILURE); + } + + execve(newargv[0], (char *const*)newargv, newenv); + fprintf(stderr, "%s: failed to run %s/virt-login-shell-helper: %s\n", + argv[0], LIBEXECDIR, strerror(errno)); + exit(EXIT_FAILURE); +} -- 2.21.0

Now that none of the libvirt.so code will ever run in a setuid context, we can remove the virIsSUID() method. The global initializer function can just inline the check itself. The new inlined check is slightly stronger as it also looks for a setgid situation. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt.c | 13 +++---------- src/libvirt_private.syms | 1 - src/remote/remote_driver.c | 23 +++-------------------- src/util/virlog.c | 9 --------- src/util/virutil.c | 12 ------------ src/util/virutil.h | 1 - 6 files changed, 6 insertions(+), 53 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 489785cec4..161001bf48 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -250,13 +250,12 @@ virGlobalInit(void) virErrorInitialize() < 0) goto error; -#ifndef LIBVIRT_SETUID_RPC_CLIENT - if (virIsSUID()) { + if (getuid() != geteuid() || + getgid() != getegid()) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("libvirt.so is not safe to use from setuid programs")); + _("libvirt.so is not safe to use from setuid/setgid programs")); goto error; } -#endif virLogSetFromEnv(); @@ -844,12 +843,6 @@ virConnectOpenInternal(const char *name, if (name && name[0] == '\0') name = NULL; - if (!name && virIsSUID()) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("An explicit URI must be provided when setuid")); - goto failed; - } - /* Convert xen -> xen:///system for back compat */ if (name && STRCASEEQ(name, "xen")) name = "xen:///system"; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c323f679b3..8f344a07ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3295,7 +3295,6 @@ virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; virIsDevMapperDevice; -virIsSUID; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 72c2336b7a..5e6007d468 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -853,21 +853,6 @@ doRemoteOpen(virConnectPtr conn, transport = trans_unix; } - /* - * We don't want to be executing external programs in setuid mode, - * so this rules out 'ext' and 'ssh' transports. Exclude libssh - * and tls too, since we're not confident the libraries are safe - * for setuid usage. Just allow UNIX sockets, since that does - * not require any external libraries or command execution - */ - if (virIsSUID() && - transport != trans_unix) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only Unix socket URI transport is allowed in setuid mode")); - return VIR_DRV_OPEN_ERROR; - } - - /* Remote server defaults to "localhost" if not specified. */ if (conn->uri && conn->uri->port != 0) { if (virAsprintf(&port, "%d", conn->uri->port) < 0) @@ -1353,8 +1338,7 @@ remoteConnectOpen(virConnectPtr conn, * transport is listed, or transport is unix, * and uid is unprivileged then auto-spawn a daemon. */ - if (!virIsSUID() && - !conn->uri->server && + if (!conn->uri->server && (transport == NULL || STREQ(transport, "unix")) && (!autostart || STRNEQ(autostart, "0"))) { @@ -1372,9 +1356,8 @@ remoteConnectOpen(virConnectPtr conn, if (geteuid() > 0) { VIR_DEBUG("Auto-spawn user daemon instance"); rflags |= VIR_DRV_OPEN_REMOTE_USER; - if (!virIsSUID() && - (!autostart || - STRNEQ(autostart, "0"))) + if (!autostart || + STRNEQ(autostart, "0")) rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; } } diff --git a/src/util/virlog.c b/src/util/virlog.c index da433878df..6a2229ae2b 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1588,7 +1588,6 @@ virLogParseOutput(const char *src) size_t count = 0; virLogPriority prio; int dest; - bool isSUID = virIsSUID(); VIR_DEBUG("output=%s", src); @@ -1626,14 +1625,6 @@ virLogParseOutput(const char *src) goto cleanup; } - /* if running with setuid, only 'stderr' is allowed */ - if (isSUID && dest != VIR_LOG_TO_STDERR) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Running with SUID permits only destination of type " - "'stderr'")); - goto cleanup; - } - switch ((virLogDestination) dest) { case VIR_LOG_TO_STDERR: ret = virLogNewOutputToStderr(prio); diff --git a/src/util/virutil.c b/src/util/virutil.c index 84ccc1a546..4e0dbe15c4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1750,18 +1750,6 @@ const char *virGetEnvAllowSUID(const char *name) return getenv(name); /* exempt from syntax-check */ } - -/** - * virIsSUID: - * Return a true value if running setuid. Does not - * check for elevated capabilities bits. - */ -bool virIsSUID(void) -{ - return getuid() != geteuid(); -} - - static time_t selfLastChanged; time_t virGetSelfLastChanged(void) diff --git a/src/util/virutil.h b/src/util/virutil.h index 7ea702f27a..52d0c33773 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -143,7 +143,6 @@ int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); const char *virGetEnvBlockSUID(const char *name); const char *virGetEnvAllowSUID(const char *name); -bool virIsSUID(void); time_t virGetSelfLastChanged(void); -- 2.21.0

Now that 100% of libvirt code is forbidden in a SUID environment, we no longer need to worry about whether env variables are trustworthy or not. The virt-login-shell setuid program, which does not link to any libvirt code, will purge all environment variables, except $TERM, before invoking the virt-login-shell-helper program which uses libvirt. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- cfg.mk | 9 --------- src/libvirt-admin.c | 2 +- src/libvirt.c | 2 +- src/libvirt_private.syms | 2 -- src/network/leaseshelper.c | 14 ++++++------- src/qemu/qemu_firmware.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetlibsshsession.c | 2 +- src/rpc/virnettlscontext.c | 2 +- src/util/virauth.c | 2 +- src/util/vircommand.c | 2 +- src/util/virfile.c | 4 ++-- src/util/virlease.c | 4 ++-- src/util/virlog.c | 6 +++--- src/util/virsystemd.c | 8 ++++---- src/util/virutil.c | 36 ++++----------------------------- src/util/virutil.h | 3 --- src/vbox/vbox_XPCOMCGlue.c | 2 +- src/vbox/vbox_common.c | 2 +- tools/virsh.c | 2 +- tools/virt-login-shell-helper.c | 4 ++-- tools/vsh.c | 12 +++++------ 22 files changed, 41 insertions(+), 83 deletions(-) diff --git a/cfg.mk b/cfg.mk index 9130b4560b..ec09550b49 100644 --- a/cfg.mk +++ b/cfg.mk @@ -855,12 +855,6 @@ sc_prohibit_unbounded_arrays_in_rpc: halt='Arrays in XDR must have a upper limit set for <NNN>' \ $(_sc_search_regexp) -sc_prohibit_getenv: - @prohibit='\b(secure_)?getenv *\(' \ - exclude='exempt from syntax-check' \ - halt='Use virGetEnv{Allow,Block}SUID instead of getenv' \ - $(_sc_search_regexp) - sc_prohibit_atoi: @prohibit='\bato(i|f|l|ll|q) *\(' \ halt='Use virStrToLong* instead of atoi, atol, atof, atoq, atoll' \ @@ -1316,9 +1310,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \ exclude_file_name_regexp--sc_prohibit_unsigned_pid = \ ^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$ -exclude_file_name_regexp--sc_prohibit_getenv = \ - ^tests/.*\.[ch]|tools/virt-login-shell\.c$$ - exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = \ ^(src/util/virlog\.h|src/network/bridge_driver\.h)$$ diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 74dedf64d8..4ae50b188f 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -169,7 +169,7 @@ getSocketPath(virURIPtr uri) static int virAdmGetDefaultURI(virConfPtr conf, char **uristr) { - const char *defname = virGetEnvAllowSUID("LIBVIRT_ADMIN_DEFAULT_URI"); + const char *defname = getenv("LIBVIRT_ADMIN_DEFAULT_URI"); if (defname && *defname) { if (VIR_STRDUP(*uristr, defname) < 0) return -1; diff --git a/src/libvirt.c b/src/libvirt.c index 161001bf48..768ad348c7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -774,7 +774,7 @@ virConnectGetDefaultURI(virConfPtr conf, char **name) { int ret = -1; - const char *defname = virGetEnvBlockSUID("LIBVIRT_DEFAULT_URI"); + const char *defname = getenv("LIBVIRT_DEFAULT_URI"); if (defname && *defname) { VIR_DEBUG("Using LIBVIRT_DEFAULT_URI '%s'", defname); if (VIR_STRDUP(*name, defname) < 0) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 983fe93d99..12c506a87a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3270,8 +3270,6 @@ virFormatIntDecimal; virFormatIntPretty; virGetDeviceID; virGetDeviceUnprivSGIO; -virGetEnvAllowSUID; -virGetEnvBlockSUID; virGetGroupID; virGetGroupList; virGetGroupName; diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index 2a10fbf33a..481f29aa59 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -86,10 +86,10 @@ main(int argc, char **argv) const char *ip = NULL; const char *mac = NULL; const char *leases_str = NULL; - const char *iaid = virGetEnvAllowSUID("DNSMASQ_IAID"); - const char *clientid = virGetEnvAllowSUID("DNSMASQ_CLIENT_ID"); - const char *interface = virGetEnvAllowSUID("DNSMASQ_INTERFACE"); - const char *hostname = virGetEnvAllowSUID("DNSMASQ_SUPPLIED_HOSTNAME"); + const char *iaid = getenv("DNSMASQ_IAID"); + const char *clientid = getenv("DNSMASQ_CLIENT_ID"); + const char *interface = getenv("DNSMASQ_INTERFACE"); + const char *hostname = getenv("DNSMASQ_SUPPLIED_HOSTNAME"); char *server_duid = NULL; int action = -1; int pid_file_fd = -1; @@ -131,7 +131,7 @@ main(int argc, char **argv) * events for expired leases. So, libvirtd sets another env var for this * purpose */ if (!interface && - !(interface = virGetEnvAllowSUID("VIR_BRIDGE_NAME"))) + !(interface = getenv("VIR_BRIDGE_NAME"))) goto cleanup; ip = argv[3]; @@ -148,11 +148,11 @@ main(int argc, char **argv) /* Check if it is an IPv6 lease */ if (iaid) { - mac = virGetEnvAllowSUID("DNSMASQ_MAC"); + mac = getenv("DNSMASQ_MAC"); clientid = argv[2]; } - if (VIR_STRDUP(server_duid, virGetEnvAllowSUID("DNSMASQ_SERVER_DUID")) < 0) + if (VIR_STRDUP(server_duid, getenv("DNSMASQ_SERVER_DUID")) < 0) goto cleanup; if (virAsprintf(&custom_lease_file, diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index bf29b10b9a..983a7c83b2 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -986,7 +986,7 @@ qemuFirmwareFetchConfigs(char ***firmwares, * much sense to parse files in root's home directory. It * makes sense only for session daemon which runs under * regular user. */ - if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0) + if (VIR_STRDUP(xdgConfig, getenv("XDG_CONFIG_HOME")) < 0) return -1; if (!xdgConfig) { diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 5e6007d468..76ea49ed8e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1297,7 +1297,7 @@ remoteConnectOpen(virConnectPtr conn, struct private_data *priv; int ret = VIR_DRV_OPEN_ERROR; int rflags = 0; - const char *autostart = virGetEnvBlockSUID("LIBVIRT_AUTOSTART"); + const char *autostart = getenv("LIBVIRT_AUTOSTART"); char *driver = NULL; char *transport = NULL; diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 093ac29071..62cbe1e06a 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -172,7 +172,7 @@ virNetLibsshSessionOnceInit(void) ssh_set_log_level(TRACE_LIBSSH); #endif - dbgLevelStr = virGetEnvAllowSUID("LIBVIRT_LIBSSH_DEBUG"); + dbgLevelStr = getenv("LIBVIRT_LIBSSH_DEBUG"); if (dbgLevelStr && virStrToLong_i(dbgLevelStr, NULL, 10, &dbgLevel) >= 0) ssh_set_log_level(dbgLevel); diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 4adc409c0b..416c8308ed 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -1439,7 +1439,7 @@ void virNetTLSSessionDispose(void *obj) void virNetTLSInit(void) { const char *gnutlsdebug; - if ((gnutlsdebug = virGetEnvAllowSUID("LIBVIRT_GNUTLS_DEBUG")) != NULL) { + if ((gnutlsdebug = getenv("LIBVIRT_GNUTLS_DEBUG")) != NULL) { int val; if (virStrToLong_i(gnutlsdebug, NULL, 10, &val) < 0) val = 10; diff --git a/src/util/virauth.c b/src/util/virauth.c index e5994cbb7c..9de3996e92 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -42,7 +42,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri, char **path) { size_t i; - const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE"); + const char *authenv = getenv("LIBVIRT_AUTH_FILE"); VIR_AUTOFREE(char *) userdir = NULL; *path = NULL; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index ea9a9fd622..79e1e87964 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1424,7 +1424,7 @@ virCommandAddEnvPass(virCommandPtr cmd, const char *name) if (!cmd || cmd->has_error) return; - value = virGetEnvAllowSUID(name); + value = getenv(name); if (value) virCommandAddEnvPair(cmd, name, value); } diff --git a/src/util/virfile.c b/src/util/virfile.c index 775192ff00..7667c16d27 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1674,7 +1674,7 @@ virFindFileInPath(const char *file) } /* copy PATH env so we can tweak it */ - origpath = virGetEnvBlockSUID("PATH"); + origpath = getenv("PATH"); if (!origpath) origpath = "/bin:/usr/bin"; @@ -1735,7 +1735,7 @@ virFileFindResourceFull(const char *filename, const char *envname) { char *ret = NULL; - const char *envval = envname ? virGetEnvBlockSUID(envname) : NULL; + const char *envval = envname ? getenv(envname) : NULL; const char *path; if (!prefix) diff --git a/src/util/virlease.c b/src/util/virlease.c index 93ca72e3af..87e9a3ce88 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -213,13 +213,13 @@ virLeaseNew(virJSONValuePtr *lease_ret, const char *server_duid) { VIR_AUTOPTR(virJSONValue) lease_new = NULL; - const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES"); + const char *exptime_tmp = getenv("DNSMASQ_LEASE_EXPIRES"); long long expirytime = 0; VIR_AUTOFREE(char *) exptime = NULL; /* In case hostname is still unknown, use the last known one */ if (!hostname) - hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME"); + hostname = getenv("DNSMASQ_OLD_HOSTNAME"); if (!mac) return 0; diff --git a/src/util/virlog.c b/src/util/virlog.c index 6a2229ae2b..4c76fbc5a4 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1308,13 +1308,13 @@ virLogSetFromEnv(void) if (virLogInitialize() < 0) return; - debugEnv = virGetEnvAllowSUID("LIBVIRT_DEBUG"); + debugEnv = getenv("LIBVIRT_DEBUG"); if (debugEnv && *debugEnv) virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv)); - debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS"); + debugEnv = getenv("LIBVIRT_LOG_FILTERS"); if (debugEnv && *debugEnv) virLogSetFilters(debugEnv); - debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS"); + debugEnv = getenv("LIBVIRT_LOG_OUTPUTS"); if (debugEnv && *debugEnv) virLogSetOutputs(debugEnv); } diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 1cb8874403..26b751311f 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -509,7 +509,7 @@ virSystemdNotifyStartup(void) .msg_iovlen = 1, }; - if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) { + if (!(path = getenv("NOTIFY_SOCKET"))) { VIR_DEBUG("Skipping systemd notify, not requested"); return; } @@ -798,7 +798,7 @@ virSystemdGetListenFDs(void) VIR_DEBUG("Setting up networking from caller"); - if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) { + if (!(pidstr = getenv("LISTEN_PID"))) { VIR_DEBUG("No LISTEN_PID from caller"); return 0; } @@ -814,7 +814,7 @@ virSystemdGetListenFDs(void) return 0; } - if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) { + if (!(fdstr = getenv("LISTEN_FDS"))) { VIR_DEBUG("No LISTEN_FDS from caller"); return 0; } @@ -866,7 +866,7 @@ virSystemdActivationNew(virSystemdActivationMap *map, if (!(act->fds = virHashCreate(10, virSystemdActivationEntryFree))) goto error; - fdnames = virGetEnvAllowSUID("LISTEN_FDNAMES"); + fdnames = getenv("LISTEN_FDNAMES"); if (fdnames) { if (virSystemdActivationInitFromNames(act, nfds, fdnames) < 0) goto error; diff --git a/src/util/virutil.c b/src/util/virutil.c index 4e0dbe15c4..89d2cf011f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -739,7 +739,7 @@ char *virGetUserShell(uid_t uid) static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) { - const char *path = virGetEnvBlockSUID(xdgenvname); + const char *path = getenv(xdgenvname); char *ret = NULL; char *home = NULL; @@ -767,7 +767,7 @@ char *virGetUserCacheDirectory(void) char *virGetUserRuntimeDirectory(void) { - const char *path = virGetEnvBlockSUID("XDG_RUNTIME_DIR"); + const char *path = getenv("XDG_RUNTIME_DIR"); if (!path || !path[0]) { return virGetUserCacheDirectory(); @@ -1137,7 +1137,7 @@ virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED) const char *dir; char *ret; - dir = virGetEnvBlockSUID("HOME"); + dir = getenv("HOME"); /* Only believe HOME if it is an absolute path and exists */ if (dir) { @@ -1157,7 +1157,7 @@ virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED) if (!dir) /* USERPROFILE is probably the closest equivalent to $HOME? */ - dir = virGetEnvBlockSUID("USERPROFILE"); + dir = getenv("USERPROFILE"); if (VIR_STRDUP(ret, dir) < 0) return NULL; @@ -1722,34 +1722,6 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) return rc; } - -/** - * virGetEnvBlockSUID: - * @name: the environment variable name - * - * Obtain an environment variable which is unsafe to - * use when running setuid. If running setuid, a NULL - * value will be returned - */ -const char *virGetEnvBlockSUID(const char *name) -{ - return secure_getenv(name); /* exempt from syntax-check */ -} - - -/** - * virGetEnvAllowSUID: - * @name: the environment variable name - * - * Obtain an environment variable which is safe to - * use when running setuid. The value will be returned - * even when running setuid - */ -const char *virGetEnvAllowSUID(const char *name) -{ - return getenv(name); /* exempt from syntax-check */ -} - static time_t selfLastChanged; time_t virGetSelfLastChanged(void) diff --git a/src/util/virutil.h b/src/util/virutil.h index 52d0c33773..b64a85f49e 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -141,9 +141,6 @@ char *virGetUnprivSGIOSysfsPath(const char *path, int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); -const char *virGetEnvBlockSUID(const char *name); -const char *virGetEnvAllowSUID(const char *name); - time_t virGetSelfLastChanged(void); void virUpdateSelfLastChanged(const char *path); diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c index 2a054f02d6..72ae49b6b2 100644 --- a/src/vbox/vbox_XPCOMCGlue.c +++ b/src/vbox/vbox_XPCOMCGlue.c @@ -190,7 +190,7 @@ VBoxCGlueInit(unsigned int *version) "/usr/local/lib/VirtualBox", "/Applications/VirtualBox.app/Contents/MacOS" }; - const char *home = virGetEnvBlockSUID("VBOX_APP_HOME"); + const char *home = getenv("VBOX_APP_HOME"); /* If the user specifies the location, try only that. */ if (home != NULL) { diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 8a912da50c..6a4ef01e64 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3558,7 +3558,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; if (VIR_STRDUP(graphics->data.desktop.display, - virGetEnvBlockSUID("DISPLAY")) < 0) + getenv("DISPLAY")) < 0) goto cleanup; } diff --git a/tools/virsh.c b/tools/virsh.c index f09ce1ec98..692a1ff16d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -913,7 +913,7 @@ main(int argc, char **argv) if (!ctl->connname) ctl->connname = vshStrdup(ctl, - virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI")); + getenv("VIRSH_DEFAULT_CONNECT_URI")); if (!ctl->imode) { ret = vshCommandRun(ctl, ctl->cmd); diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c index 8dc3bedaa0..d062c07a27 100644 --- a/tools/virt-login-shell-helper.c +++ b/tools/virt-login-shell-helper.c @@ -372,8 +372,8 @@ main(int argc, char **argv) /* We're duping the string because the clearenv() * call will shortly release the pointer we get - * back from virGetEnvAllowSUID() right here */ - if (VIR_STRDUP(term, virGetEnvAllowSUID("TERM")) < 0) + * back from getenv() right here */ + if (VIR_STRDUP(term, getenv("TERM")) < 0) goto cleanup; /* A fork is required to create new process in correct pid namespace. */ diff --git a/tools/vsh.c b/tools/vsh.c index 5de082cb34..9bdd90e362 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2429,7 +2429,7 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc) int fd; char ebuf[1024]; - tmpdir = virGetEnvBlockSUID("TMPDIR"); + tmpdir = getenv("TMPDIR"); if (!tmpdir) tmpdir = "/tmp"; if (virAsprintf(&ret, "%s/virshXXXXXX.xml", tmpdir) < 0) { vshError(ctl, "%s", _("out of memory")); @@ -2476,9 +2476,9 @@ vshEditFile(vshControl *ctl, const char *filename) int outfd = STDOUT_FILENO; int errfd = STDERR_FILENO; - editor = virGetEnvBlockSUID("VISUAL"); + editor = getenv("VISUAL"); if (!editor) - editor = virGetEnvBlockSUID("EDITOR"); + editor = getenv("EDITOR"); if (!editor) editor = DEFAULT_EDITOR; @@ -2956,7 +2956,7 @@ vshReadlineInit(vshControl *ctl) goto cleanup; /* Limit the total size of the history buffer */ - if ((histsize_str = virGetEnvBlockSUID(histsize_env))) { + if ((histsize_str = getenv(histsize_env))) { if (virStrToLong_i(histsize_str, NULL, 10, &max_history) < 0) { vshError(ctl, _("Bad $%s value."), histsize_env); goto cleanup; @@ -3072,7 +3072,7 @@ vshInitDebug(vshControl *ctl) return -1; /* log level not set from commandline, check env variable */ - debugEnv = virGetEnvAllowSUID(env); + debugEnv = getenv(env); if (debugEnv) { int debug; if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 || @@ -3091,7 +3091,7 @@ vshInitDebug(vshControl *ctl) return -1; /* log file not set from cmdline */ - debugEnv = virGetEnvBlockSUID(env); + debugEnv = getenv(env); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); vshOpenLogFile(ctl); -- 2.21.0

Use the plain libc APIs to avoid a dependancy on the main libvirt code from the nss module. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/nss/libvirt_nss.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index f50dec48ba..d057827ebc 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -41,7 +41,6 @@ #include "virfile.h" #include "virtime.h" #include "virerror.h" -#include "virstring.h" #include "virsocketaddr.h" #include "configmake.h" #include "virmacmap.h" @@ -193,13 +192,20 @@ findLeaseInJSON(leaseAddress **tmpAddress, } if (macs) { + const char **macstmp = macs; const char *macAddr; + bool match = false; macAddr = virJSONValueObjectGetString(lease, "mac-address"); if (!macAddr) continue; - if (!virStringListHasString(macs, macAddr)) + while (*macstmp && !match) { + if (STREQ(*macstmp, macAddr)) + match = true; + macstmp++; + } + if (!match) continue; } else { const char *lease_name; @@ -295,8 +301,9 @@ findLease(const char *name, DEBUG("Dir: %s", leaseDir); while ((entry = readdir(dir)) != NULL) { char *path; + size_t dlen = strlen(entry->d_name); - if (virStringHasSuffix(entry->d_name, ".status")) { + if (dlen >= 7 && STREQ(entry->d_name + dlen - 7, ".status")) { if (!(path = virFileBuildPath(leaseDir, entry->d_name, NULL))) goto cleanup; @@ -307,7 +314,7 @@ findLease(const char *name, goto cleanup; } VIR_FREE(path); - } else if (virStringHasSuffix(entry->d_name, ".macs")) { + } else if (dlen >= 5 && STREQ(entry->d_name + dlen - 5, ".macs")) { if (!(path = virFileBuildPath(leaseDir, entry->d_name, NULL))) goto cleanup; -- 2.21.0

Build a list of mac addresses immediately, so that later code searching for leases can be simplified and avoid needing to use the virMacMap object. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- cfg.mk | 4 +-- tools/nss/libvirt_nss.c | 73 ++++++++++++++++++++++++----------------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/cfg.mk b/cfg.mk index f4cd215abc..8c352d7b9a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1226,7 +1226,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c|tools/virt-login-shell\.c|tools/nss/libvirt_nss\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ - ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$) + ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c|tools/nss/libvirt_nss\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c))$$) @@ -1259,7 +1259,7 @@ exclude_file_name_regexp--sc_prohibit_canonicalize_file_name = \ ^(cfg\.mk|tests/virfilemock\.c)$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c)$$ + ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss\.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index ed2ad956e9..a849b8e5f7 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -274,8 +274,10 @@ findLease(const char *name, ssize_t nleases; VIR_AUTOFREE(leaseAddress *) tmpAddress = NULL; size_t ntmpAddress = 0; - VIR_AUTOFREE(virMacMapPtr *) macmaps = NULL; - size_t nMacmaps = 0; + virMacMapPtr map = NULL; + char **macs = NULL; + size_t nmacs = 0; + size_t i; *address = NULL; *naddress = 0; @@ -313,23 +315,43 @@ findLease(const char *name, goto cleanup; } VIR_FREE(path); +#if defined(LIBVIRT_NSS_GUEST) } else if (dlen >= 5 && STREQ(entry->d_name + dlen - 5, ".macs")) { + const char * const *newmacs; if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) goto cleanup; - if (VIR_REALLOC_N_QUIET(macmaps, nMacmaps + 1) < 0) { - VIR_FREE(path); - goto cleanup; - } - DEBUG("Processing %s", path); - if (!(macmaps[nMacmaps] = virMacMapNew(path))) { + if (!(map = virMacMapNew(path))) { ERROR("Unable to parse %s", path); VIR_FREE(path); goto cleanup; } - nMacmaps++; VIR_FREE(path); + + DEBUG("Looking up macs in %p for %s", map, name); + newmacs = virMacMapLookup(map, name); + for (i = 0; newmacs && newmacs[i] != NULL; i++) + ; + + DEBUG("Got %zu macs", i); + if (i > 0) { + if (VIR_REALLOC_N_QUIET(macs, nmacs + i + 1) < 0) + goto cleanup; + + for (i = 0; newmacs[i] != NULL; i++) { + char *macdup; + if (!(macdup = strdup(newmacs[i]))) + goto cleanup; + DEBUG("Capture mac %s", macdup); + macs[nmacs++] = macdup; + } + macs[nmacs] = NULL; + } + + virObjectUnref(map); + map = NULL; +#endif /* LIBVIRT_NSS_GUEST */ } errno = 0; @@ -340,29 +362,18 @@ findLease(const char *name, nleases = virJSONValueArraySize(leases_array); DEBUG("Read %zd leases", nleases); -#if !defined(LIBVIRT_NSS_GUEST) +#if defined(LIBVIRT_NSS_GUEST) + DEBUG("Finding with %zu macs", nmacs); + if (!nmacs) + goto cleanup; +#endif + if (findLeaseInJSON(&tmpAddress, &ntmpAddress, leases_array, nleases, - name, NULL, af, found) < 0) + name, (const char**)macs, af, found) < 0) goto cleanup; -#else /* defined(LIBVIRT_NSS_GUEST) */ - - size_t i; - for (i = 0; i < nMacmaps; i++) { - const char **macs = (const char **) virMacMapLookup(macmaps[i], name); - - if (!macs) - continue; - - if (findLeaseInJSON(&tmpAddress, &ntmpAddress, - leases_array, nleases, - name, macs, af, found) < 0) - goto cleanup; - } - -#endif /* defined(LIBVIRT_NSS_GUEST) */ - + DEBUG("Found %zu addresses", ntmpAddress); sortAddr(tmpAddress, ntmpAddress); VIR_STEAL_PTR(*address, tmpAddress); @@ -372,11 +383,13 @@ findLease(const char *name, ret = 0; cleanup: + virObjectUnref(map); *errnop = errno; + for (i = 0; i < nmacs; i++) + free(macs[i]); + free(macs); if (dir) closedir(dir); - while (nMacmaps) - virObjectUnref(macmaps[--nMacmaps]); return ret; } -- 2.21.0

The .leases file is currently loaded using the virLease class, which in turn uses the virJSON parsing code. This pulls in a heap of libvirt code (logging, hash tables, etc) which we do not wish to depend on. This uses the yajl parser code directly, so the only dep is yajl and plain libc functions. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- cfg.mk | 6 +- tools/Makefile.am | 6 +- tools/nss/libvirt_nss.c | 207 +++-------------- tools/nss/libvirt_nss_leases.c | 399 +++++++++++++++++++++++++++++++++ tools/nss/libvirt_nss_leases.h | 40 ++++ tools/nss/libvirt_nss_macs.c | 4 +- tools/nss/libvirt_nss_macs.h | 2 + 7 files changed, 481 insertions(+), 183 deletions(-) create mode 100644 tools/nss/libvirt_nss_leases.c create mode 100644 tools/nss/libvirt_nss_leases.h diff --git a/cfg.mk b/cfg.mk index 33bf29c5b0..cc1f79a051 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1226,10 +1226,10 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c|tools/virt-login-shell\.c|tools/nss/libvirt_nss\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ - ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c|tools/nss/libvirt_nss_macs\.c$$) + ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c|tools/nss/libvirt_nss_(leases|macs)\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ - (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_macs\.c)$$) + (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^tests/(virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) @@ -1259,7 +1259,7 @@ exclude_file_name_regexp--sc_prohibit_canonicalize_file_name = \ ^(cfg\.mk|tests/virfilemock\.c)$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_macs)?\.c)$$ + ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c|tools/nss/libvirt_nss(_leases|_macs)?\.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ diff --git a/tools/Makefile.am b/tools/Makefile.am index eee4226231..61812a2cb1 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -476,7 +476,10 @@ endif ! WITH_BSD_NSS LIBVIRT_NSS_SOURCES = \ nss/libvirt_nss.c \ - nss/libvirt_nss.h + nss/libvirt_nss.h \ + nss/libvirt_nss_leases.c \ + nss/libvirt_nss_leases.h \ + $(NULL) if WITH_NSS noinst_LTLIBRARIES += nss/libnss_libvirt_impl.la @@ -485,6 +488,7 @@ nss_libnss_libvirt_impl_la_SOURCES = \ nss_libnss_libvirt_impl_la_CFLAGS = \ -DLIBVIRT_NSS \ + $(YAJL_CFLAGS) \ $(AM_CFLAGS) \ $(NULL) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index b3756b984a..47d2ba9435 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -36,12 +36,13 @@ # include <nsswitch.h> #endif -#include "virlease.h" #include "viralloc.h" #include "virtime.h" #include "virsocketaddr.h" #include "configmake.h" +#include "libvirt_nss_leases.h" + #if defined(LIBVIRT_NSS_GUEST) # include "libvirt_nss_macs.h" #endif /* !LIBVIRT_NSS_GUEST */ @@ -51,13 +52,6 @@ #define LIBVIRT_ALIGN(x) (((x) + __SIZEOF_POINTER__ - 1) & ~(__SIZEOF_POINTER__ - 1)) #define FAMILY_ADDRESS_SIZE(family) ((family) == AF_INET6 ? 16 : 4) -typedef struct { - unsigned char addr[16]; - int af; - long long expirytime; -} leaseAddress; - - static int leaseAddressSorter(const void *a, const void *b) @@ -77,147 +71,6 @@ sortAddr(leaseAddress *tmpAddress, } -static int -appendAddr(const char *name ATTRIBUTE_UNUSED, - leaseAddress **tmpAddress, - size_t *ntmpAddress, - virJSONValuePtr lease, - int af) -{ - const char *ipAddr; - virSocketAddr sa; - int family; - long long expirytime; - size_t i; - - if (!(ipAddr = virJSONValueObjectGetString(lease, "ip-address"))) { - ERROR("ip-address field missing for %s", name); - return -1; - } - - DEBUG("IP address: %s", ipAddr); - - if (virSocketAddrParse(&sa, ipAddr, AF_UNSPEC) < 0) { - ERROR("Unable to parse %s", ipAddr); - return -1; - } - - family = VIR_SOCKET_ADDR_FAMILY(&sa); - if (af != AF_UNSPEC && af != family) { - DEBUG("Skipping address which family is %d, %d requested", family, af); - return 0; - } - - if (virJSONValueObjectGetNumberLong(lease, "expiry-time", &expirytime) < 0) { - /* A lease cannot be present without expiry-time */ - ERROR("expiry-time field missing for %s", name); - return -1; - } - - for (i = 0; i < *ntmpAddress; i++) { - if (memcmp((*tmpAddress)[i].addr, - (family == AF_INET ? - (void *) &sa.data.inet4.sin_addr.s_addr : - (void *) &sa.data.inet6.sin6_addr.s6_addr), - FAMILY_ADDRESS_SIZE(family)) == 0) { - DEBUG("IP address already in the list"); - return 0; - } - } - - if (VIR_REALLOC_N_QUIET(*tmpAddress, *ntmpAddress + 1) < 0) { - ERROR("Out of memory"); - return -1; - } - - (*tmpAddress)[*ntmpAddress].expirytime = expirytime; - (*tmpAddress)[*ntmpAddress].af = family; - memcpy((*tmpAddress)[*ntmpAddress].addr, - (family == AF_INET ? - (void *) &sa.data.inet4.sin_addr.s_addr : - (void *) &sa.data.inet6.sin6_addr.s6_addr), - FAMILY_ADDRESS_SIZE(family)); - (*ntmpAddress)++; - return 0; -} - - -static int -findLeaseInJSON(leaseAddress **tmpAddress, - size_t *ntmpAddress, - virJSONValuePtr leases_array, - size_t nleases, - const char *name, - const char **macs, - size_t nmacs, - int af, - bool *found) -{ - size_t i; - size_t j; - long long expirytime; - time_t currtime; - - if ((currtime = time(NULL)) == (time_t) - 1) { - ERROR("Failed to get current system time"); - return -1; - } - - for (i = 0; i < nleases; i++) { - virJSONValuePtr lease = virJSONValueArrayGet(leases_array, i); - - if (!lease) { - /* This should never happen (TM) */ - ERROR("Unable to get element %zu of %zu", i, nleases); - return -1; - } - - if (macs) { - const char *macAddr; - bool match = false; - - macAddr = virJSONValueObjectGetString(lease, "mac-address"); - if (!macAddr) - continue; - - for (j = 0; j < nmacs && !match; j++) { - if (STREQ(macs[j], macAddr)) - match = true; - } - if (!match) - continue; - } else { - const char *lease_name; - - lease_name = virJSONValueObjectGetString(lease, "hostname"); - - if (STRNEQ_NULLABLE(name, lease_name)) - continue; - } - - if (virJSONValueObjectGetNumberLong(lease, "expiry-time", &expirytime) < 0) { - /* A lease cannot be present without expiry-time */ - ERROR("expiry-time field missing for %s", name); - return -1; - } - - /* Do not report expired lease */ - if (expirytime < (long long) currtime) { - DEBUG("Skipping expired lease for %s", name); - continue; - } - - DEBUG("Found record for %s", name); - *found = true; - - if (appendAddr(name, tmpAddress, ntmpAddress, lease, af) < 0) - return -1; - } - - return 0; -} - - /** * findLease: * @name: domain name to lookup @@ -250,13 +103,12 @@ findLease(const char *name, int ret = -1; const char *leaseDir = LEASEDIR; struct dirent *entry; - VIR_AUTOPTR(virJSONValue) leases_array = NULL; - ssize_t nleases; - VIR_AUTOFREE(leaseAddress *) tmpAddress = NULL; - size_t ntmpAddress = 0; + char **leaseFiles = NULL; + size_t nleaseFiles = 0; char **macs = NULL; size_t nmacs = 0; size_t i; + time_t now; *address = NULL; *naddress = 0; @@ -273,27 +125,21 @@ findLease(const char *name, goto cleanup; } - if (!(leases_array = virJSONValueNewArray())) { - ERROR("Failed to create json array"); - goto cleanup; - } - DEBUG("Dir: %s", leaseDir); while ((entry = readdir(dir)) != NULL) { char *path; size_t dlen = strlen(entry->d_name); if (dlen >= 7 && STREQ(entry->d_name + dlen - 7, ".status")) { + char **tmpLease; if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) goto cleanup; - DEBUG("Processing %s", path); - if (virLeaseReadCustomLeaseFile(leases_array, path, NULL, NULL) < 0) { - ERROR("Unable to parse %s", path); - VIR_FREE(path); + tmpLease = realloc(leaseFiles, sizeof(char *) * (nleaseFiles + 1)); + if (!tmpLease) goto cleanup; - } - VIR_FREE(path); + leaseFiles = tmpLease; + leaseFiles[nleaseFiles++] = path; #if defined(LIBVIRT_NSS_GUEST) } else if (dlen >= 5 && STREQ(entry->d_name + dlen - 5, ".macs")) { if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) @@ -313,9 +159,6 @@ findLease(const char *name, closedir(dir); dir = NULL; - nleases = virJSONValueArraySize(leases_array); - DEBUG("Read %zd leases", nleases); - #if defined(LIBVIRT_NSS_GUEST) DEBUG("Finding with %zu macs", nmacs); if (!nmacs) @@ -324,26 +167,38 @@ findLease(const char *name, DEBUG(" %s", macs[i]); #endif - if (findLeaseInJSON(&tmpAddress, &ntmpAddress, - leases_array, nleases, - name, (const char**)macs, nmacs, - af, found) < 0) + if ((now = time(NULL)) == (time_t)-1) { + DEBUG("Failed to get time"); goto cleanup; + } - DEBUG("Found %zu addresses", ntmpAddress); - sortAddr(tmpAddress, ntmpAddress); + for (i = 0; i < nleaseFiles; i++) { + if (findLeases(leaseFiles[i], + name, macs, nmacs, + af, now, + address, naddress, + found) < 0) + goto cleanup; + } - VIR_STEAL_PTR(*address, tmpAddress); - *naddress = ntmpAddress; - ntmpAddress = 0; + DEBUG("Found %zu addresses", *naddress); + sortAddr(*address, *naddress); ret = 0; cleanup: *errnop = errno; + for (i = 0; i < nleaseFiles; i++) + free(leaseFiles[i]); + free(leaseFiles); for (i = 0; i < nmacs; i++) free(macs[i]); free(macs); + if (ret < 0) { + free(*address); + *address = NULL; + *naddress = 0; + } if (dir) closedir(dir); return ret; diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c new file mode 100644 index 0000000000..44089af313 --- /dev/null +++ b/tools/nss/libvirt_nss_leases.c @@ -0,0 +1,399 @@ +/* + * libvirt_nss_leases.c: Name Service Switch plugin lease file parser + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include <unistd.h> +#include <string.h> +#include <stdlib.h> +#include <fcntl.h> + +#include <yajl/yajl_gen.h> +#include <yajl/yajl_parse.h> + +#include "libvirt_nss_leases.h" +#include "libvirt_nss.h" +#include "virsocketaddr.h" +#include "viralloc.h" + +enum { + FIND_LEASES_STATE_START, + FIND_LEASES_STATE_LIST, + FIND_LEASES_STATE_ENTRY, +}; + + +typedef struct { + const char *name; + char **macs; + size_t nmacs; + int state; + unsigned long long now; + int af; + bool *found; + leaseAddress **addrs; + size_t *naddrs; + + char *key; + struct { + unsigned long long expiry; + char *ipaddr; + char *macaddr; + char *hostname; + } entry; +} findLeasesParser; + + +static int +appendAddr(const char *name ATTRIBUTE_UNUSED, + leaseAddress **tmpAddress, + size_t *ntmpAddress, + const char *ipAddr, + long long expirytime, + int af) +{ + virSocketAddr sa; + int family; + size_t i; + + DEBUG("IP address: %s", ipAddr); + if (virSocketAddrParse(&sa, ipAddr, AF_UNSPEC) < 0) { + ERROR("Unable to parse %s", ipAddr); + return -1; + } + + family = VIR_SOCKET_ADDR_FAMILY(&sa); + if (af != AF_UNSPEC && af != family) { + DEBUG("Skipping address which family is %d, %d requested", family, af); + return 0; + } + + for (i = 0; i < *ntmpAddress; i++) { + if (family == AF_INET) { + if (memcmp((*tmpAddress)[i].addr, + &sa.data.inet4.sin_addr.s_addr, + sizeof(sa.data.inet4.sin_addr.s_addr)) == 0) { + DEBUG("IP address already in the list"); + return 0; + } + } else { + if (memcmp((*tmpAddress)[i].addr, + &sa.data.inet6.sin6_addr.s6_addr, + sizeof(sa.data.inet6.sin6_addr.s6_addr)) == 0) { + DEBUG("IP address already in the list"); + return 0; + } + } + } + + if (VIR_REALLOC_N_QUIET(*tmpAddress, *ntmpAddress + 1) < 0) { + ERROR("Out of memory"); + return -1; + } + + (*tmpAddress)[*ntmpAddress].expirytime = expirytime; + (*tmpAddress)[*ntmpAddress].af = family; + if (family == AF_INET) + memcpy((*tmpAddress)[*ntmpAddress].addr, + &sa.data.inet4.sin_addr.s_addr, + sizeof(sa.data.inet4.sin_addr.s_addr)); + else + memcpy((*tmpAddress)[*ntmpAddress].addr, + &sa.data.inet6.sin6_addr.s6_addr, + sizeof(sa.data.inet6.sin6_addr.s6_addr)); + (*ntmpAddress)++; + return 0; +} + + +static int +findLeasesParserInteger(void *ctx, + long long val) +{ + findLeasesParser *parser = ctx; + + DEBUG("Parse int state=%d '%lld' (map key '%s')", + parser->state, val, NULLSTR(parser->key)); + if (!parser->key) + return 0; + + if (parser->state == FIND_LEASES_STATE_ENTRY) { + if (STRNEQ(parser->key, "expiry-time")) + return 0; + + parser->entry.expiry = val; + } else { + return 0; + } + return 1; +} + + +static int +findLeasesParserString(void *ctx, + const unsigned char *stringVal, + size_t stringLen) +{ + findLeasesParser *parser = ctx; + + DEBUG("Parse string state=%d '%.*s' (map key '%s')", + parser->state, (int)stringLen, (const char *)stringVal, + NULLSTR(parser->key)); + if (!parser->key) + return 0; + + if (parser->state == FIND_LEASES_STATE_ENTRY) { + if (STREQ(parser->key, "ip-address")) { + if (!(parser->entry.ipaddr = strndup((char *)stringVal, stringLen))) + return 0; + } else if (STREQ(parser->key, "mac-address")) { + if (!(parser->entry.macaddr = strndup((char *)stringVal, stringLen))) + return 0; + } else if (STREQ(parser->key, "hostname")) { + if (!(parser->entry.hostname = strndup((char *)stringVal, stringLen))) + return 0; + } else { + return 0; + } + } else { + return 0; + } + return 1; +} + + +static int +findLeasesParserMapKey(void *ctx, + const unsigned char *stringVal, + size_t stringLen) +{ + findLeasesParser *parser = ctx; + + DEBUG("Parse map key state=%d '%.*s'", + parser->state, (int)stringLen, (const char *)stringVal); + + free(parser->key); + if (!(parser->key = strndup((char *)stringVal, stringLen))) + return 0; + + return 1; +} + + +static int +findLeasesParserStartMap(void *ctx) +{ + findLeasesParser *parser = ctx; + + DEBUG("Parse start map state=%d", parser->state); + + if (parser->state != FIND_LEASES_STATE_LIST) + return 0; + + free(parser->key); + parser->key = NULL; + parser->state = FIND_LEASES_STATE_ENTRY; + + return 1; +} + + +static int +findLeasesParserEndMap(void *ctx) +{ + findLeasesParser *parser = ctx; + size_t i; + bool found = false; + + DEBUG("Parse end map state=%d", parser->state); + + if (parser->entry.macaddr == NULL) + return 0; + + if (parser->state != FIND_LEASES_STATE_ENTRY) + return 0; + + if (parser->nmacs) { + DEBUG("Check %zu macs", parser->nmacs); + for (i = 0; i < parser->nmacs && !found; i++) { + DEBUG("Check mac '%s' vs '%s'", parser->macs[i], NULLSTR(parser->entry.macaddr)); + if (STREQ_NULLABLE(parser->macs[i], parser->entry.macaddr)) + found = true; + } + } else { + DEBUG("Check name '%s' vs '%s'", parser->name, NULLSTR(parser->entry.hostname)); + if (STREQ_NULLABLE(parser->name, parser->entry.hostname)) + found = true; + } + DEBUG("Found %d", found); + if (parser->entry.expiry < parser->now) { + DEBUG("Entry expired at %llu vs now %llu", + parser->entry.expiry, parser->now); + found = false; + } + if (!parser->entry.ipaddr) + found = false; + + if (found) { + *parser->found = true; + + if (appendAddr(parser->name, + parser->addrs, parser->naddrs, + parser->entry.ipaddr, + parser->entry.expiry, + parser->af) < 0) + return 0; + } + + free(parser->entry.macaddr); + free(parser->entry.ipaddr); + free(parser->entry.hostname); + parser->entry.macaddr = NULL; + parser->entry.ipaddr = NULL; + parser->entry.hostname = NULL; + + parser->state = FIND_LEASES_STATE_LIST; + + return 1; +} + + +static int +findLeasesParserStartArray(void *ctx) +{ + findLeasesParser *parser = ctx; + + DEBUG("Parse start array state=%d", parser->state); + + if (parser->state == FIND_LEASES_STATE_START) { + parser->state = FIND_LEASES_STATE_LIST; + } else { + return 0; + } + + return 1; +} + + +static int +findLeasesParserEndArray(void *ctx) +{ + findLeasesParser *parser = ctx; + + DEBUG("Parse end array state=%d", parser->state); + + if (parser->state == FIND_LEASES_STATE_LIST) + parser->state = FIND_LEASES_STATE_START; + else + return 0; + + return 1; +} + + +int +findLeases(const char *file, + const char *name, + char **macs, + size_t nmacs, + int af, + time_t now, + leaseAddress **addrs, + size_t *naddrs, + bool *found) +{ + int fd = -1; + int ret = -1; + const yajl_callbacks parserCallbacks = { + NULL, /* null */ + NULL, /* bool */ + findLeasesParserInteger, + NULL, /* double */ + NULL, /* number */ + findLeasesParserString, + findLeasesParserStartMap, + findLeasesParserMapKey, + findLeasesParserEndMap, + findLeasesParserStartArray, + findLeasesParserEndArray, + }; + findLeasesParser parserState = { + .name = name, + .macs = macs, + .nmacs = nmacs, + .af = af, + .now = now, + .found = found, + .addrs = addrs, + .naddrs = naddrs, + }; + yajl_handle parser; + char line[1024]; + int rv; + + if ((fd = open(file, O_RDONLY)) < 0) { + ERROR("Cannot open %s", file); + goto cleanup; + } + + parser = yajl_alloc(&parserCallbacks, NULL, &parserState); + if (!parser) { + ERROR("Unable to create JSON parser"); + goto cleanup; + } + + while (1) { + rv = read(fd, line, sizeof(line)); + if (rv < 0) + goto cleanup; + if (rv == 0) + break; + + if (yajl_parse(parser, (const unsigned char *)line, rv) != + yajl_status_ok) { + ERROR("Parse failed %s", + yajl_get_error(parser, 1, + (const unsigned char*)line, rv)); + goto cleanup; + } + } + + if (yajl_complete_parse(parser) != yajl_status_ok) { + ERROR("Parse failed %s", + yajl_get_error(parser, 1, NULL, 0)); + goto cleanup; + } + + ret = 0; + + cleanup: + if (ret != 0) { + free(*addrs); + *addrs = NULL; + *naddrs = 0; + } + free(parserState.entry.ipaddr); + free(parserState.entry.macaddr); + free(parserState.entry.hostname); + free(parserState.key); + if (fd != -1) + close(fd); + return ret; +} diff --git a/tools/nss/libvirt_nss_leases.h b/tools/nss/libvirt_nss_leases.h new file mode 100644 index 0000000000..e213681e46 --- /dev/null +++ b/tools/nss/libvirt_nss_leases.h @@ -0,0 +1,40 @@ +/* + * libvirt_nss_leases.h: Name Service Switch plugin lease file parser + * + * Copyright (C) 2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +typedef struct { + unsigned char addr[16]; + int af; + long long expirytime; +} leaseAddress; + +int +findLeases(const char *file, + const char *name, + char **macs, + size_t nmacs, + int af, + time_t now, + leaseAddress **addrs, + size_t *naddrs, + bool *found); diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c index 0d0b6b1eaa..9fe5b83e86 100644 --- a/tools/nss/libvirt_nss_macs.c +++ b/tools/nss/libvirt_nss_macs.c @@ -28,10 +28,8 @@ #include <yajl/yajl_gen.h> #include <yajl/yajl_parse.h> -#include "internal.h" - -#include "libvirt_nss.h" #include "libvirt_nss_macs.h" +#include "libvirt_nss.h" enum { FIND_MACS_STATE_START, diff --git a/tools/nss/libvirt_nss_macs.h b/tools/nss/libvirt_nss_macs.h index c504a8cf1f..64e291f549 100644 --- a/tools/nss/libvirt_nss_macs.h +++ b/tools/nss/libvirt_nss_macs.h @@ -20,6 +20,8 @@ #pragma once +#include "internal.h" + int findMACs(const char *file, const char *name, -- 2.21.0

Use the plain libc APIs to avoid a dependancy on the main libvirt code from the nss module. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/nss/libvirt_nss.c | 16 +++++++++++----- tools/nss/libvirt_nss.h | 5 +++-- tools/nss/libvirt_nss_leases.c | 6 ++++-- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 2719e19cda..a9814cf0dc 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -31,13 +31,15 @@ #include <sys/types.h> #include <dirent.h> #include <arpa/inet.h> +#include <stdlib.h> +#include <string.h> +#include <time.h> + #if defined(HAVE_BSD_NSS) # include <nsswitch.h> #endif -#include "viralloc.h" -#include "virtime.h" #include "configmake.h" #include "libvirt_nss_leases.h" @@ -146,10 +148,10 @@ findLease(const char *name, DEBUG("Processing %s", path); if (findMACs(path, name, &macs, &nmacs) < 0) { - VIR_FREE(path); + free(path); goto cleanup; } - VIR_FREE(path); + free(path); #endif /* LIBVIRT_NSS_GUEST */ } @@ -243,7 +245,7 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, { enum nss_status ret = NSS_STATUS_UNAVAIL; char *r_name, **r_aliases, *r_addr, *r_addr_next, **r_addr_list; - VIR_AUTOFREE(leaseAddress *) addr = NULL; + leaseAddress *addr = NULL; size_t naddr, i; bool found = false; size_t nameLen, need, idx = 0; @@ -259,6 +261,7 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, af = AF_INET; if ((r = findLease(name, af, &addr, &naddr, &found, errnop)) < 0) { + free(addr); /* Error occurred. Return immediately. */ if (*errnop == EAGAIN) { *herrnop = TRY_AGAIN; @@ -273,11 +276,13 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, /* NOT found */ *errnop = ESRCH; *herrnop = HOST_NOT_FOUND; + free(addr); return NSS_STATUS_NOTFOUND; } else if (!naddr) { /* Found, but no data */ *errnop = ENXIO; *herrnop = NO_DATA; + free(addr); return NSS_STATUS_UNAVAIL; } @@ -349,6 +354,7 @@ NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, ret = NSS_STATUS_SUCCESS; cleanup: + free(addr); return ret; } diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index 6e4be125d2..fa4ff892c6 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -30,13 +30,14 @@ #if 0 -# include "virerror.h" +# include <errno.h> # define ERROR(...) \ do { \ char ebuf[1024]; \ + strerror_r(errno, ebuf, sizeof(ebuf)); \ fprintf(stderr, "ERROR %s:%d : ", __FUNCTION__, __LINE__); \ fprintf(stderr, __VA_ARGS__); \ - fprintf(stderr, " : %s\n", virStrerror(errno, ebuf, sizeof(ebuf))); \ + fprintf(stderr, " : %s\n", ebuf); \ fprintf(stderr, "\n"); \ } while (0) diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index 803b14cc55..ddd50288d2 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -30,7 +30,6 @@ #include "libvirt_nss_leases.h" #include "libvirt_nss.h" -#include "viralloc.h" enum { FIND_LEASES_STATE_START, @@ -79,6 +78,7 @@ appendAddr(const char *name ATTRIBUTE_UNUSED, } sa; unsigned char addr[16]; int err; + leaseAddress *newAddr; DEBUG("IP address: %s", ipAddr); @@ -131,10 +131,12 @@ appendAddr(const char *name ATTRIBUTE_UNUSED, } } - if (VIR_REALLOC_N_QUIET(*tmpAddress, *ntmpAddress + 1) < 0) { + newAddr = realloc(*tmpAddress, sizeof(*newAddr) * (*ntmpAddress + 1)); + if (!newAddr) { ERROR("Out of memory"); return -1; } + *tmpAddress = newAddr; (*tmpAddress)[*ntmpAddress].expirytime = expirytime; (*tmpAddress)[*ntmpAddress].af = family; -- 2.21.0

On 8/1/19 5:00 PM, Daniel P. Berrangé wrote:

Very nice cleanup, which also makes NSS library smaller in size (I mean those .so.2 files). I've found some mem leaks and mis-alignments, though. Please squash this into respective patches: diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index a9814cf0dc..7122065b28 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -527,7 +527,7 @@ aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip) (*aip)->ai_next = res0; while ((*aip)->ai_next) - *aip = (*aip)->ai_next; + *aip = (*aip)->ai_next; addrList++; } diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c index ddd50288d2..48a54d5841 100644 --- a/tools/nss/libvirt_nss_leases.c +++ b/tools/nss/libvirt_nss_leases.c @@ -108,7 +108,7 @@ appendAddr(const char *name ATTRIBUTE_UNUSED, return 0; } - if (af != AF_UNSPEC && af != family) { + if (af != AF_UNSPEC && af != family) { DEBUG("Skipping address which family is %d, %d requested", family, af); return 0; } @@ -374,7 +374,7 @@ findLeases(const char *file, .addrs = addrs, .naddrs = naddrs, }; - yajl_handle parser; + yajl_handle parser = NULL; char line[1024]; int rv; @@ -419,6 +419,7 @@ findLeases(const char *file, *addrs = NULL; *naddrs = 0; } + yajl_free(parser); free(parserState.entry.ipaddr); free(parserState.entry.macaddr); free(parserState.entry.hostname); diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c index 9fe5b83e86..fb5526bd7b 100644 --- a/tools/nss/libvirt_nss_macs.c +++ b/tools/nss/libvirt_nss_macs.c @@ -55,8 +55,8 @@ typedef struct { static int findMACsParserString(void *ctx, - const unsigned char *stringVal, - size_t stringLen) + const unsigned char *stringVal, + size_t stringLen) { findMACsParser *parser = ctx; @@ -70,6 +70,7 @@ findMACsParserString(void *ctx, if (STRNEQ(parser->key, "domain")) return 0; + free(parser->entry.name); if (!(parser->entry.name = strndup((char *)stringVal, stringLen))) return 0; } else if (parser->state == FIND_MACS_STATE_ENTRY_MACS) { @@ -93,8 +94,8 @@ findMACsParserString(void *ctx, static int findMACsParserMapKey(void *ctx, - const unsigned char *stringVal, - size_t stringLen) + const unsigned char *stringVal, + size_t stringLen) { findMACsParser *parser = ctx; @@ -226,7 +227,7 @@ findMACs(const char *file, .macs = macs, .nmacs = nmacs, }; - yajl_handle parser; + yajl_handle parser = NULL; char line[1024]; size_t i; int rv; @@ -276,6 +277,7 @@ findMACs(const char *file, *macs = NULL; *nmacs = 0; } + yajl_free(parser); for (i = 0; i < parserState.entry.nmacs; i++) free(parserState.entry.macs[i]); free(parserState.entry.macs); With that: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal