10:24 a.m.
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