Without this patch, at least tests/daemon-conf (which sticks $builddir/src in the PATH) tries to execute the directory $builddir/src/qemu rather than the real /usr/bin/qemu binary. That was fine when qemu_capabilities silently ignored execution failure, but not so good once it is converted to use virCommand. * src/util/util.h (virFileExists): Adjust prototype. (virFileIsExecutable): New prototype. * src/util/util.c (virFindFileInPath): Reject non-executables and directories. Avoid huge stack allocation. (virFileExists): Use lighter-weight syscall. (virFileIsExecutable): New function. * src/libvirt_private.syms (util.h): Export new function. --- Questions: Should I be requiring S_ISREG, rather than !S_ISDIR (that is, should we be rejecting devices and sockets as non-exectuable)? Should I import the gnulib module euidaccess (and/or faccessat) for the access check? Using access(F_OK) is okay regardless of uid/gid, but access(X_OK) may have different answers than euidaccess(X_OK) when the effective uid/gid do not match the current uid/gid. However, dragging in the gnulib module will require adding an extra link library for some platforms (for example, Solaris needs -lgen), which means the change is more invasive as it will also affect Makefiles. src/libvirt_private.syms | 1 + src/util/util.c | 53 ++++++++++++++++++++++++++++++--------------- src/util/util.h | 7 +++-- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65911df..948bbe1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -850,6 +850,7 @@ virFileDeletePid; virFileExists; virFileFindMountPoint; virFileHasSuffix; +virFileIsExecutable; virFileLinkPointsTo; virFileMakePath; virFileMatchesNameSuffix; diff --git a/src/util/util.c b/src/util/util.c index 60feb79..25e6185 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1255,7 +1255,7 @@ int virFileResolveLink(const char *linkpath, } /* - * Finds a requested file in the PATH env. e.g.: + * Finds a requested executable file in the PATH env. e.g.: * "kvm-img" will return "/usr/bin/kvm-img" * * You must free the result @@ -1263,19 +1263,18 @@ int virFileResolveLink(const char *linkpath, char *virFindFileInPath(const char *file) { char *path; - char pathenv[PATH_MAX]; - char *penv = pathenv; + char *pathiter; char *pathseg; - char fullpath[PATH_MAX]; + char *fullpath = NULL; if (file == NULL) return NULL; /* if we are passed an absolute path (starting with /), return a - * copy of that path + * copy of that path, after validating that it is executable */ - if (file[0] == '/') { - if (virFileExists(file)) + if (IS_ABSOLUTE_FILE_NAME(file)) { + if (virFileIsExecutable(file)) return strdup(file); else return NULL; @@ -1284,27 +1283,45 @@ char *virFindFileInPath(const char *file) /* copy PATH env so we can tweak it */ path = getenv("PATH"); - if (path == NULL || virStrcpyStatic(pathenv, path) == NULL) + if (path == NULL || (path = strdup(path)) == NULL) return NULL; /* for each path segment, append the file to search for and test for * it. return it if found. */ - while ((pathseg = strsep(&penv, ":")) != NULL) { - snprintf(fullpath, PATH_MAX, "%s/%s", pathseg, file); - if (virFileExists(fullpath)) - return strdup(fullpath); + pathiter = path; + while ((pathseg = strsep(&pathiter, ":")) != NULL) { + if (virAsprintf(&fullpath, "%s/%s", pathseg, file) < 0 || + virFileIsExecutable(fullpath)) + break; + VIR_FREE(fullpath); } - return NULL; + VIR_FREE(path); + return fullpath; } -int virFileExists(const char *path) + +bool virFileExists(const char *path) { - struct stat st; + return access(path, F_OK) == 0; +} - if (stat(path, &st) >= 0) - return(1); - return(0); +/* Check that a file can be executed by the current user, and is not + * a directory. */ +bool +virFileIsExecutable(const char *file) +{ + struct stat sb; + + /* The existence of ACLs means that checking (sb.st_mode&0111) for + * executable bits may give false results; plus, access is + * lighter-weight than stat for a first pass filter. + * + * XXX: should this use gnulib's euidaccess module? + */ + return (access(file, X_OK) == 0 && + stat(file, &sb) == 0 && + !S_ISDIR(sb.st_mode)); } #ifndef WIN32 diff --git a/src/util/util.h b/src/util/util.h index 989962f..54c3058 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -1,8 +1,7 @@ - /* * utils.h: common, generic utility functions * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -32,6 +31,7 @@ # include <sys/select.h> # include <sys/types.h> # include <stdarg.h> +# include <stdbool.h> # ifndef MIN # define MIN(a, b) ((a) < (b) ? (a) : (b)) @@ -120,7 +120,8 @@ int virFileResolveLink(const char *linkpath, char *virFindFileInPath(const char *file); -int virFileExists(const char *path); +bool virFileExists(const char *file); +bool virFileIsExecutable(const char *file); char *virFileSanitizePath(const char *path); -- 1.7.3.4