Re: [libvirt] [PATCH] virFindFileInPath: only find executable non-directory
On Wed, Jan 12, 2011 at 09:24:57AM -0700, Eric Blake wrote:
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.
Not looking at your patch, I have to question why on earth
the script is adding $builddir/src to the $PATH ? There are
no command line binaries in src/ anymore, only in tools/
and it clearly doesn't need any of them if it hasn't also
added tools/ to $PATH.
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.
+/* 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));
If we're doing stat() anyway, it could be better to kill
the access() check and just check S_IXUSR on sb.sb_mode.
Yes, that isn't the same as access, but I think we it can
be argued that we should accept binaries which have any
'x' bit set even if we ourselves can't execute them.
eg, if /usr/bin/qemu was -rwx-r--r-- then we should
not skip it. We should pick that binary on the basis
that the user will probably want to see the EACCESS
message when we later try to exec() it, so they can
see the installation bug & fix it.
If we skipped the binary, they'd get a 'cannot find
binary foo' error message from libvirt which would
be somewhat misleading because it would clearly exist
as far as the user can see.
Regards,
Daniel