* test -x should use faccessat, not stat @ 2010-02-10 15:32 Eric Blake 2010-02-14 6:11 ` Herbert Xu 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2010-02-10 15:32 UTC (permalink / raw) To: dash This report was originally raised on the cygwin list: http://cygwin.com/ml/cygwin/2010-02/msg00239.html In short, in the presence of ACLs, dash's implementation of test -r, test -w, and test -x gives incorrect answers, when the current user has permissions to access a file that were granted by ACLs but not by the current stat() permissions. dash should be using faccessat(,AT_EACCESS) (or eaccess/euidaccess) if available, rather than stat(), to determine whether a file is accessible. -- Eric Blake ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: test -x should use faccessat, not stat 2010-02-10 15:32 test -x should use faccessat, not stat Eric Blake @ 2010-02-14 6:11 ` Herbert Xu 2010-02-15 13:31 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Herbert Xu @ 2010-02-14 6:11 UTC (permalink / raw) To: Eric Blake; +Cc: dash Eric Blake <ebb9@byu.net> wrote: > This report was originally raised on the cygwin list: > > http://cygwin.com/ml/cygwin/2010-02/msg00239.html > > In short, in the presence of ACLs, dash's implementation of test -r, test -w, > and test -x gives incorrect answers, when the current user has permissions to > access a file that were granted by ACLs but not by the current stat() > permissions. dash should be using faccessat(,AT_EACCESS) (or > eaccess/euidaccess) if available, rather than stat(), to determine whether a > file is accessible. What does bash to in this case? Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: test -x should use faccessat, not stat 2010-02-14 6:11 ` Herbert Xu @ 2010-02-15 13:31 ` Eric Blake 2010-02-16 12:04 ` Herbert Xu 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2010-02-15 13:31 UTC (permalink / raw) To: Herbert Xu; +Cc: dash According to Herbert Xu on 2/13/2010 11:11 PM: > Eric Blake <ebb9@byu.net> wrote: >> This report was originally raised on the cygwin list: >> >> http://cygwin.com/ml/cygwin/2010-02/msg00239.html >> >> In short, in the presence of ACLs, dash's implementation of test -r, test -w, >> and test -x gives incorrect answers, when the current user has permissions to >> access a file that were granted by ACLs but not by the current stat() >> permissions. dash should be using faccessat(,AT_EACCESS) (or >> eaccess/euidaccess) if available, rather than stat(), to determine whether a >> file is accessible. > > What does bash to in this case? The bash source code shows the following: In test.c, unary_test() calls sh_eaccess for test -r, -w, and -x. In lib/sh/eaccess.c, bash currently uses: int sh_eaccess (path, mode) char *path; int mode; { if (path_is_devfd (path)) return (sh_stataccess (path, mode)); #if defined (HAVE_EACCESS) /* FreeBSD */ return (eaccess (path, mode)); #elif defined (EFF_ONLY_OK) /* SVR4(?), SVR4.2 */ return access (path, mode|EFF_ONLY_OK); #else if (mode == F_OK) return (sh_stataccess (path, mode)); # if HAVE_DECL_SETREGID if (current_user.uid != current_user.euid || current_user.gid != current_user.egid) return (sh_euidaccess (path, mode)); # endif if (current_user.uid == current_user.euid && current_user.gid == current_user.egid) return (access (path, mode)); return (sh_stataccess (path, mode)); #endif } But this could probably be improved to take advantage of the standardized faccessat(path,mode,AT_EACCESS) in the case where that exists. Furthermore, the link to the post on the cygwin list shows that bash, zsh, and pdksh all honored ACLs, and that dash is the odd man out for not recognizing when the current user has rights due to ACLs that are not visible through the stat mode bits. Finally, it is worth pointing out that on at least cygwin, faccessat and friends are faster than stat. Do you want me to prepare the patch? -- Don't work too hard, make some time for fun as well! Eric Blake ebb9@byu.net ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: test -x should use faccessat, not stat 2010-02-15 13:31 ` Eric Blake @ 2010-02-16 12:04 ` Herbert Xu 2010-04-02 14:03 ` Herbert Xu 0 siblings, 1 reply; 6+ messages in thread From: Herbert Xu @ 2010-02-16 12:04 UTC (permalink / raw) To: Eric Blake; +Cc: dash On Mon, Feb 15, 2010 at 06:31:14AM -0700, Eric Blake wrote: > > Finally, it is worth pointing out that on at least cygwin, faccessat and > friends are faster than stat. Do you want me to prepare the patch? First of all I completely agree with you that the current dash behaviour is busted. As to how we should fix it, let me think about this a little more before making a decision. Thanks, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: test -x should use faccessat, not stat 2010-02-16 12:04 ` Herbert Xu @ 2010-04-02 14:03 ` Herbert Xu 2010-04-02 14:57 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Herbert Xu @ 2010-04-02 14:03 UTC (permalink / raw) To: Eric Blake; +Cc: dash On Tue, Feb 16, 2010 at 08:04:13PM +0800, Herbert Xu wrote: > > First of all I completely agree with you that the current dash > behaviour is busted. As to how we should fix it, let me think > about this a little more before making a decision. After much deliberation (alright, I've simply been busy elsewhere :) I've committed this patch. commit 1d68712ba2e439f36874c4ed1e3d9ffec177a06c Author: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri Apr 2 22:02:22 2010 +0800 [BUILTIN] Use faccessat if available Eric Blake suggested that we should use faccessat so that ACLs and other corner cases are handled correctly. This patch does exactly that. Note that faccessat doesn't handle ACLs when euid != uid, as this case is currently implemented by glibc instead of the kernel, using code similar to the existing dash test. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/ChangeLog b/ChangeLog index 7af5070..9f63819 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2010-04-02 Herbert Xu <herbert@gondor.apana.org.au> + * Use faccessat if available. + +2010-04-02 Herbert Xu <herbert@gondor.apana.org.au> + * Make trap signal name/number errors non-fatal. * Release 0.5.6. diff --git a/configure.ac b/configure.ac index df6e099..c943725 100644 --- a/configure.ac +++ b/configure.ac @@ -46,7 +46,8 @@ dnl Checks for header files. AC_CHECK_HEADERS(alloca.h) dnl Checks for library functions. -AC_CHECK_FUNCS(bsearch getpwnam getrlimit imaxdiv isalpha killpg mempcpy \ +AC_CHECK_FUNCS(bsearch faccessat getpwnam getrlimit imaxdiv isalpha killpg \ + mempcpy \ sigsetmask stpcpy strchrnul strsignal strtod strtoimax \ strtoumax sysconf) diff --git a/src/bltin/test.c b/src/bltin/test.c index 8e7077a..7888f38 100644 --- a/src/bltin/test.c +++ b/src/bltin/test.c @@ -11,6 +11,7 @@ #include <sys/stat.h> #include <sys/types.h> +#include <fcntl.h> #include <stdint.h> #include <stdlib.h> #include <string.h> @@ -147,8 +148,12 @@ static int isoperand(char **); static int newerf(const char *, const char *); static int olderf(const char *, const char *); static int equalf(const char *, const char *); +#ifdef HAVE_FACCESSAT +static int test_file_access(const char *, int); +#else static int test_st_mode(const struct stat64 *, int); static int bash_group_member(gid_t); +#endif static inline intmax_t getn(const char *s) { @@ -295,6 +300,14 @@ primary(enum token n) return strlen(*t_wp) != 0; case FILTT: return isatty(getn(*t_wp)); +#ifdef HAVE_FACCESSAT + case FILRD: + return test_file_access(*t_wp, R_OK); + case FILWR: + return test_file_access(*t_wp, W_OK); + case FILEX: + return test_file_access(*t_wp, X_OK); +#endif default: return filstat(*t_wp, n); } @@ -364,12 +377,14 @@ filstat(char *nm, enum token mode) return 0; switch (mode) { +#ifndef HAVE_FACCESSAT case FILRD: return test_st_mode(&s, R_OK); case FILWR: return test_st_mode(&s, W_OK); case FILEX: return test_st_mode(&s, X_OK); +#endif case FILEXIST: return 1; case FILREG: @@ -469,6 +484,12 @@ equalf (const char *f1, const char *f2) b1.st_ino == b2.st_ino); } +#ifdef HAVE_FACCESSAT +static int test_file_access(const char *path, int mode) +{ + return !faccessat(AT_FDCWD, path, mode, AT_EACCESS); +} +#else /* HAVE_FACCESSAT */ /* * Similar to what access(2) does, but uses the effective uid and gid. * Doesn't make the mistake of telling root that any file is executable. @@ -519,3 +540,4 @@ bash_group_member(gid_t gid) return (0); } +#endif /* HAVE_FACCESSAT */ Thanks, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: test -x should use faccessat, not stat 2010-04-02 14:03 ` Herbert Xu @ 2010-04-02 14:57 ` Eric Blake 0 siblings, 0 replies; 6+ messages in thread From: Eric Blake @ 2010-04-02 14:57 UTC (permalink / raw) To: Herbert Xu; +Cc: dash According to Herbert Xu on 4/2/2010 8:03 AM: > After much deliberation (alright, I've simply been busy elsewhere :) > I've committed this patch. > > commit 1d68712ba2e439f36874c4ed1e3d9ffec177a06c > Note that faccessat doesn't handle ACLs when euid != uid, as > this case is currently implemented by glibc instead of the kernel, > using code similar to the existing dash test. That faccessat bug is only true for current Linux kernels. Cygwin faccessat does the correct thing, even when euid != uid. Thanks for applying this. -- Don't work too hard, make some time for fun as well! Eric Blake ebb9@byu.net ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-02 15:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-10 15:32 test -x should use faccessat, not stat Eric Blake 2010-02-14 6:11 ` Herbert Xu 2010-02-15 13:31 ` Eric Blake 2010-02-16 12:04 ` Herbert Xu 2010-04-02 14:03 ` Herbert Xu 2010-04-02 14:57 ` Eric Blake
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.