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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list