From 391c397e48ac6cefcc065c75236c1ef7d3a152e2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 18 Mar 2011 14:41:13 -0600 Subject: [PATCH] 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(,R_OK) is only needed to execute a script file; a binary only needs access(,X_OK) (besides, it's unusual to see a file with x but not r permissions, whether script or binary). * 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. --- 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 | 22 ++++++++-------------- 6 files changed, 25 insertions(+), 27 deletions(-) diff --git a/cfg.mk b/cfg.mk index 2a97f88e95..ac419f74a8 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 c30620a63b..fb933a23fd 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 d8aa9cdc0c..f86e7f5efb 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 40da5c233d..b05bbcbecc 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 f19a4a8202..551448743a 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 62446eadbb..99dddc4c05 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 @@ -94,13 +94,12 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { char *path; - struct stat sb; int ret; if (driver == NULL) { virHookReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid hook name for #%d"), no); - return(-1); + return -1; } ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, driver); @@ -108,24 +107,19 @@ virHookCheck(int no, const char *driver) { virHookReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to build path for %s hook"), driver); - return(-1); + return -1; } - if (stat(path, &sb) < 0) { + if (!virFileIsExecutable(path)) { ret = 0; - VIR_DEBUG("No hook script %s", path); + VIR_WARN("Missing or non-executable hook script %s", path); } else { - if ((access(path, X_OK) != 0) || (!S_ISREG(sb.st_mode))) { - ret = 0; - VIR_WARN("Non executable hook script %s", path); - } else { - ret = 1; - VIR_DEBUG("Found hook script %s", path); - } + ret = 1; + VIR_DEBUG("Found hook script %s", path); } VIR_FREE(path); - return(ret); + return ret; } /*