On 01/12/2011 10:39 AM, Daniel P. Berrange wrote:
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.
tests/Makefile.am is the culprit for that PATH setting: path_add = $$abs_top_builddir/src$(PATH_SEPARATOR)$$abs_top_builddir/daemon$(PATH_SEPARATOR)$$abs_top_builddir/tools TESTS_ENVIRONMENT = \ ... PATH="$(path_add)$(PATH_SEPARATOR)$$PATH" \ Probably worth a separate cleanup, now that you mention it.
+/* 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.
Or, conversely, if the go+x bits are set ACLs include u-x (and thus deny the current user the right to 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.
Fair enough - I'll rewrite the patch to avoid access(X_OK), on the grounds that ACLs are corner case enough that we don't mind getting the occasional wrong answer due to ACL weirdness. Which means:
Questions:
Should I be requiring S_ISREG, rather than !S_ISDIR (that is, should we be rejecting devices and sockets as non-exectuable)?
Still not answered, but I'll go ahead and make that change for v2.
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.
Answered - avoid access (and thus euidaccess) altogether, and go with the naive but usually right stat() parsing instead. v2 coming shortly. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org