Re: [libvirt] [PATCH] virFindFileInPath: only find executable non-directory
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)