Re: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)

This simplifies several callers that were repeating checks already guaranteed by util.c, and makes other callers more robust to now reject directories. remote_driver.c was over-strict - access(,X_OK) is not strictly needed to execute a file (although its unusual to see a file with X_OK but not R_OK).

* cfg.mk (sc_prohibit_access_xok): New syntax-check rule. (exclude_file_name_regexp--sc_prohibit_access_xok): Exempt one use. * src/network/bridge_driver.c (networkStartRadvd): Fix offenders. * src/qemu/qemu_capabilities.c (qemuCapsProbeMachineTypes) (qemuCapsInitGuest, qemuCapsInit, qemuCapsExtractVersionInfo): Likewise. * src/remote/remote_driver.c (remoteFindDaemonPath): Likewise. * src/uml/uml_driver.c (umlStartVMDaemon): Likewise. * src/util/hooks.c (virHookCheck): Likewise. --- This patch depends on the unreviewed: https://www.redhat.com/archives/libvir-list/2011-March/msg00770.html cfg.mk | 9 +++++++++ src/network/bridge_driver.c | 2 +- src/qemu/qemu_capabilities.c | 15 +++++---------- src/remote/remote_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/util/hooks.c | 4 ++-- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/cfg.mk b/cfg.mk index 2a97f88..ac419f7 100644 --- a/cfg.mk +++ b/cfg.mk @@ -258,6 +258,13 @@ sc_prohibit_fork_wrappers: halt='use virCommand for child processes' \ $(_sc_search_regexp) +# access with X_OK accepts directories, but we can't exec() those. +# access with F_OK or R_OK is okay, though. +sc_prohibit_access_xok: + @prohibit='access''(at)? *\(.*X_OK' \ + halt='use virFileIsExecutable instead of access''(,X_OK)' \ + $(_sc_search_regexp) + # Similar to the gnulib maint.mk rule for sc_prohibit_strcmp # Use STREQLEN or STRPREFIX rather than comparing strncmp == 0, or != 0. sc_prohibit_strncmp: @@ -551,6 +558,8 @@ exclude_file_name_regexp--sc_po_check = ^docs/ exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ ^(include/libvirt/virterror\.h|daemon/dispatch\.c|src/util/virterror\.c)$$ +exclude_file_name_regexp--sc_prohibit_access_xok = ^src/util/util\.c$$ + exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \ (^docs|^python/(libvirt-override|typewrappers)\.c$$) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c30620a..fb933a2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -689,7 +689,7 @@ networkStartRadvd(virNetworkObjPtr network) network->radvdPid = -1; - if (access(RADVD, X_OK)< 0) { + if (!virFileIsExecutable(RADVD)) { virReportSystemError(errno, _("Cannot find %s - " "Possibly the package isn't installed"), diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d8aa9cd..f86e7f5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -178,7 +178,7 @@ qemuCapsProbeMachineTypes(const char *binary, * Technically we could catch the exec() failure, but that's * in a sub-process so it's hard to feed back a useful error. */ - if (access(binary, X_OK)< 0) { + if (!virFileIsExecutable(binary)) { virReportSystemError(errno, _("Cannot find QEMU binary %s"), binary); return -1; } @@ -451,12 +451,9 @@ qemuCapsInitGuest(virCapsPtr caps, */ binary = virFindFileInPath(info->binary); - if (binary == NULL || access(binary, X_OK) != 0) { + if (binary == NULL || !virFileIsExecutable(binary)) { VIR_FREE(binary); binary = virFindFileInPath(info->altbinary); - - if (binary != NULL&& access(binary, X_OK) != 0) - VIR_FREE(binary); } /* Can use acceleration for KVM/KQEMU if @@ -475,10 +472,8 @@ qemuCapsInitGuest(virCapsPtr caps, for (i = 0; i< ARRAY_CARDINALITY(kvmbins); ++i) { kvmbin = virFindFileInPath(kvmbins[i]); - if (kvmbin == NULL || access(kvmbin, X_OK) != 0) { - VIR_FREE(kvmbin); + if (!kvmbin) continue; - } haskvm = 1; if (!binary) @@ -753,7 +748,7 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps) /* Then possibly the Xen paravirt guests (ie Xenner */ xenner = virFindFileInPath("xenner"); - if (xenner != NULL&& access(xenner, X_OK) == 0&& + if (xenner != NULL&& virFileIsExecutable(xenner) == 0&& access("/dev/kvm", F_OK) == 0) { for (i = 0 ; i< ARRAY_CARDINALITY(arch_info_xen) ; i++) /* Allow Xen 32-on-32, 32-on-64 and 64-on-64 */ @@ -1147,7 +1142,7 @@ int qemuCapsExtractVersionInfo(const char *qemu, const char *arch, * Technically we could catch the exec() failure, but that's * in a sub-process so it's hard to feed back a useful error. */ - if (access(qemu, X_OK)< 0) { + if (!virFileIsExecutable(qemu)) { virReportSystemError(errno, _("Cannot find QEMU binary %s"), qemu); return -1; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b844d9a..a8fb52d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -306,7 +306,7 @@ remoteFindDaemonPath(void) return(customDaemon); for (i = 0; serverPaths[i]; i++) { - if (access(serverPaths[i], X_OK | R_OK) == 0) { + if (virFileIsExecutable(serverPaths[i])) { return serverPaths[i]; } } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 538d5f7..2af53ff 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -829,7 +829,7 @@ static int umlStartVMDaemon(virConnectPtr conn, * Technically we could catch the exec() failure, but that's * in a sub-process so its hard to feed back a useful error */ - if (access(vm->def->os.kernel, X_OK)< 0) { + if (!virFileIsExecutable(vm->def->os.kernel)) { virReportSystemError(errno, _("Cannot find UML kernel %s"), vm->def->os.kernel); diff --git a/src/util/hooks.c b/src/util/hooks.c index 5ba2036..8231349 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -1,7 +1,7 @@ /* * hooks.c: implementation of the synchronous hooks support * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2010 Daniel Veillard * * This library is free software; you can redistribute it and/or @@ -113,7 +113,7 @@ virHookCheck(int no, const char *driver) { ret = 0; VIR_DEBUG("No hook script %s", path); } else { - if ((access(path, X_OK) != 0) || (!S_ISREG(sb.st_mode))) { + if (!virFileIsExecutable(path)) {

ret = 0; VIR_WARN("Non executable hook script %s", path); } else {