[libvirt] [PATCH] virFindFileInPath: only find executable non-directory

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110112/761a5f13/attachment-0001.sig>