From 35778253a5ed71e87a608ca590b63729d9f88567 Mon Sep 17 00:00:00 2001 From: Nick Kralevich Date: Tue, 24 Feb 2015 13:40:43 -0800 Subject: [PATCH] Fix "faccessat ignores flags" The kernel system call faccessat() does not have any flags arguments, so passing flags to the kernel is currently ignored. Fix the kernel system call so that no flags argument is passed in. Ensure that we don't support AT_SYMLINK_NOFOLLOW. This non-POSIX (http://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html) flag is a glibc extension, and has non-intuitive, error prone behavior. For example, consider the following code: symlink("foo.is.dangling", "foo"); if (faccessat(AT_FDCWD, "foo", R_OK, AT_SYMLINK_NOFOLLOW) == 0) { int fd = openat(AT_FDCWD, "foo", O_RDONLY | O_NOFOLLOW); } The faccessat() call in glibc will return true, but an attempt to open the dangling symlink will end up failing. GLIBC documents this as returning the access mode of the symlink itself, which will always return true for any symlink on Linux. Some further discussions of this are at: * http://lists.landley.net/pipermail/toybox-landley.net/2014-September/003617.html * http://permalink.gmane.org/gmane.linux.lib.musl.general/6952 AT_SYMLINK_NOFOLLOW seems broken by design. I suspect this is why this function was never added to POSIX. (note that "access" is pretty much broken by design too, since it introduces a race condition between check and action). We shouldn't support this until it's clearly documented by POSIX or we can have it produce intuitive results. Don't support AT_EACCESS for now. Implementing it is complicated, and pretty much useless on Android, since we don't have setuid binaries. See http://git.musl-libc.org/cgit/musl/commit/?id=0a05eace163cee9b08571d2ff9d90f5e82d9c228 for how an implementation might look. Bug: 18867827 Change-Id: I25b86c5020f3152ffa3ac3047f6c4152908d0e04 --- libc/Android.mk | 1 + libc/SYSCALLS.TXT | 2 +- .../syscalls/{faccessat.S => ___faccessat.S} | 5 +- .../syscalls/{faccessat.S => ___faccessat.S} | 5 +- .../syscalls/{faccessat.S => ___faccessat.S} | 5 +- .../syscalls/{faccessat.S => ___faccessat.S} | 5 +- .../syscalls/{faccessat.S => ___faccessat.S} | 16 ++--- .../syscalls/{faccessat.S => ___faccessat.S} | 6 +- libc/bionic/faccessat.cpp | 59 +++++++++++++++++++ tests/sys_stat_test.cpp | 40 +++++++++++++ 10 files changed, 122 insertions(+), 22 deletions(-) rename libc/arch-arm/syscalls/{faccessat.S => ___faccessat.S} (81%) rename libc/arch-arm64/syscalls/{faccessat.S => ___faccessat.S} (79%) rename libc/arch-mips/syscalls/{faccessat.S => ___faccessat.S} (82%) rename libc/arch-mips64/syscalls/{faccessat.S => ___faccessat.S} (85%) rename libc/arch-x86/syscalls/{faccessat.S => ___faccessat.S} (70%) rename libc/arch-x86_64/syscalls/{faccessat.S => ___faccessat.S} (81%) create mode 100644 libc/bionic/faccessat.cpp diff --git a/libc/Android.mk b/libc/Android.mk index 9dd386411..cb1d8c034 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -116,6 +116,7 @@ libc_bionic_ndk_src_files := \ bionic/error.cpp \ bionic/eventfd_read.cpp \ bionic/eventfd_write.cpp \ + bionic/faccessat.cpp \ bionic/fchmod.cpp \ bionic/fchmodat.cpp \ bionic/ffs.cpp \ diff --git a/libc/SYSCALLS.TXT b/libc/SYSCALLS.TXT index aae7de779..150dd14f9 100644 --- a/libc/SYSCALLS.TXT +++ b/libc/SYSCALLS.TXT @@ -130,7 +130,7 @@ int fremovexattr(int, const char*) all int __getdents64:getdents64(unsigned int, struct dirent*, unsigned int) arm,arm64,mips,mips64,x86,x86_64 int __openat:openat(int, const char*, int, mode_t) all -int faccessat(int, const char*, int, int) all +int ___faccessat:faccessat(int, const char*, int) all int ___fchmodat:fchmodat(int, const char*, mode_t) all int fchownat(int, const char*, uid_t, gid_t, int) all int fstatat64|fstatat:fstatat64(int, const char*, struct stat*, int) arm,mips,x86 diff --git a/libc/arch-arm/syscalls/faccessat.S b/libc/arch-arm/syscalls/___faccessat.S similarity index 81% rename from libc/arch-arm/syscalls/faccessat.S rename to libc/arch-arm/syscalls/___faccessat.S index a1df5c09f..1d09cf7c5 100644 --- a/libc/arch-arm/syscalls/faccessat.S +++ b/libc/arch-arm/syscalls/___faccessat.S @@ -2,7 +2,7 @@ #include -ENTRY(faccessat) +ENTRY(___faccessat) mov ip, r7 ldr r7, =__NR_faccessat swi #0 @@ -11,4 +11,5 @@ ENTRY(faccessat) bxls lr neg r0, r0 b __set_errno_internal -END(faccessat) +END(___faccessat) +.hidden ___faccessat diff --git a/libc/arch-arm64/syscalls/faccessat.S b/libc/arch-arm64/syscalls/___faccessat.S similarity index 79% rename from libc/arch-arm64/syscalls/faccessat.S rename to libc/arch-arm64/syscalls/___faccessat.S index 4c96cfa7b..6a41b697c 100644 --- a/libc/arch-arm64/syscalls/faccessat.S +++ b/libc/arch-arm64/syscalls/___faccessat.S @@ -2,7 +2,7 @@ #include -ENTRY(faccessat) +ENTRY(___faccessat) mov x8, __NR_faccessat svc #0 @@ -11,4 +11,5 @@ ENTRY(faccessat) b.hi __set_errno_internal ret -END(faccessat) +END(___faccessat) +.hidden ___faccessat diff --git a/libc/arch-mips/syscalls/faccessat.S b/libc/arch-mips/syscalls/___faccessat.S similarity index 82% rename from libc/arch-mips/syscalls/faccessat.S rename to libc/arch-mips/syscalls/___faccessat.S index e61610620..4e11bae27 100644 --- a/libc/arch-mips/syscalls/faccessat.S +++ b/libc/arch-mips/syscalls/___faccessat.S @@ -2,7 +2,7 @@ #include -ENTRY(faccessat) +ENTRY(___faccessat) .set noreorder .cpload t9 li v0, __NR_faccessat @@ -16,4 +16,5 @@ ENTRY(faccessat) j t9 nop .set reorder -END(faccessat) +END(___faccessat) +.hidden ___faccessat diff --git a/libc/arch-mips64/syscalls/faccessat.S b/libc/arch-mips64/syscalls/___faccessat.S similarity index 85% rename from libc/arch-mips64/syscalls/faccessat.S rename to libc/arch-mips64/syscalls/___faccessat.S index 18bb80062..240625f46 100644 --- a/libc/arch-mips64/syscalls/faccessat.S +++ b/libc/arch-mips64/syscalls/___faccessat.S @@ -2,7 +2,7 @@ #include -ENTRY(faccessat) +ENTRY(___faccessat) .set push .set noreorder li v0, __NR_faccessat @@ -22,4 +22,5 @@ ENTRY(faccessat) j t9 move ra, t0 .set pop -END(faccessat) +END(___faccessat) +.hidden ___faccessat diff --git a/libc/arch-x86/syscalls/faccessat.S b/libc/arch-x86/syscalls/___faccessat.S similarity index 70% rename from libc/arch-x86/syscalls/faccessat.S rename to libc/arch-x86/syscalls/___faccessat.S index 9d522311b..361a6ea6e 100644 --- a/libc/arch-x86/syscalls/faccessat.S +++ b/libc/arch-x86/syscalls/___faccessat.S @@ -2,7 +2,7 @@ #include -ENTRY(faccessat) +ENTRY(___faccessat) pushl %ebx .cfi_def_cfa_offset 8 .cfi_rel_offset ebx, 0 @@ -12,13 +12,9 @@ ENTRY(faccessat) pushl %edx .cfi_adjust_cfa_offset 4 .cfi_rel_offset edx, 0 - pushl %esi - .cfi_adjust_cfa_offset 4 - .cfi_rel_offset esi, 0 - mov 20(%esp), %ebx - mov 24(%esp), %ecx - mov 28(%esp), %edx - mov 32(%esp), %esi + mov 16(%esp), %ebx + mov 20(%esp), %ecx + mov 24(%esp), %edx movl $__NR_faccessat, %eax int $0x80 cmpl $-MAX_ERRNO, %eax @@ -28,9 +24,9 @@ ENTRY(faccessat) call __set_errno_internal addl $4, %esp 1: - popl %esi popl %edx popl %ecx popl %ebx ret -END(faccessat) +END(___faccessat) +.hidden ___faccessat diff --git a/libc/arch-x86_64/syscalls/faccessat.S b/libc/arch-x86_64/syscalls/___faccessat.S similarity index 81% rename from libc/arch-x86_64/syscalls/faccessat.S rename to libc/arch-x86_64/syscalls/___faccessat.S index 05a6e7863..e8fd3f585 100644 --- a/libc/arch-x86_64/syscalls/faccessat.S +++ b/libc/arch-x86_64/syscalls/___faccessat.S @@ -2,8 +2,7 @@ #include -ENTRY(faccessat) - movq %rcx, %r10 +ENTRY(___faccessat) movl $__NR_faccessat, %eax syscall cmpq $-MAX_ERRNO, %rax @@ -13,4 +12,5 @@ ENTRY(faccessat) call __set_errno_internal 1: ret -END(faccessat) +END(___faccessat) +.hidden ___faccessat diff --git a/libc/bionic/faccessat.cpp b/libc/bionic/faccessat.cpp new file mode 100644 index 000000000..5f375e0f7 --- /dev/null +++ b/libc/bionic/faccessat.cpp @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include +#include +#include + +extern "C" int ___faccessat(int, const char*, int); + +int faccessat(int dirfd, const char* pathname, int mode, int flags) { + // "The mode specifies the accessibility check(s) to be performed, + // and is either the value F_OK, or a mask consisting of the + // bitwise OR of one or more of R_OK, W_OK, and X_OK." + if ((mode != F_OK) && ((mode & ~(R_OK | W_OK | X_OK)) != 0) && + ((mode & (R_OK | W_OK | X_OK)) == 0)) { + errno = EINVAL; + return -1; + } + + if (flags != 0) { + // We deliberately don't support AT_SYMLINK_NOFOLLOW, a glibc + // only feature which is error prone and dangerous. + // + // AT_EACCESS isn't supported either. Android doesn't have setuid + // programs, and never runs code with euid!=uid. It could be + // implemented in an expensive way, following the model at + // https://gitlab.com/bminor/musl/commit/0a05eace163cee9b08571d2ff9d90f5e82d9c228 + // but not worth it. + errno = EINVAL; + return -1; + } + + return ___faccessat(dirfd, pathname, mode); +} diff --git a/tests/sys_stat_test.cpp b/tests/sys_stat_test.cpp index 7bbb7c665..28c7c5262 100644 --- a/tests/sys_stat_test.cpp +++ b/tests/sys_stat_test.cpp @@ -219,3 +219,43 @@ TEST(sys_stat, fchmodat_AT_SYMLINK_NOFOLLOW_with_dangling_symlink) { ASSERT_EQ(ENOTSUP, errno); unlink(linkname); } + +TEST(sys_stat, faccessat_EINVAL) { + ASSERT_EQ(-1, faccessat(AT_FDCWD, "/dev/null", F_OK, ~AT_SYMLINK_NOFOLLOW)); + ASSERT_EQ(EINVAL, errno); +#if defined(__BIONIC__) + ASSERT_EQ(-1, faccessat(AT_FDCWD, "/dev/null", ~(R_OK | W_OK | X_OK), 0)); + ASSERT_EQ(EINVAL, errno); +#else + ASSERT_EQ(0, faccessat(AT_FDCWD, "/dev/null", ~(R_OK | W_OK | X_OK), AT_SYMLINK_NOFOLLOW)); + ASSERT_EQ(-1, faccessat(AT_FDCWD, "/dev/null", ~(R_OK | W_OK | X_OK), 0)); + ASSERT_EQ(EINVAL, errno); +#endif +} + +TEST(sys_stat, faccessat_AT_SYMLINK_NOFOLLOW_EINVAL) { +#if defined(__BIONIC__) + // Android doesn't support AT_SYMLINK_NOFOLLOW + ASSERT_EQ(-1, faccessat(AT_FDCWD, "/dev/null", F_OK, AT_SYMLINK_NOFOLLOW)); + ASSERT_EQ(EINVAL, errno); +#else + ASSERT_EQ(0, faccessat(AT_FDCWD, "/dev/null", F_OK, AT_SYMLINK_NOFOLLOW)); +#endif +} + +TEST(sys_stat, faccessat_dev_null) { + ASSERT_EQ(0, faccessat(AT_FDCWD, "/dev/null", F_OK, 0)); + ASSERT_EQ(0, faccessat(AT_FDCWD, "/dev/null", R_OK, 0)); + ASSERT_EQ(0, faccessat(AT_FDCWD, "/dev/null", W_OK, 0)); + ASSERT_EQ(0, faccessat(AT_FDCWD, "/dev/null", R_OK|W_OK, 0)); +} + +TEST(sys_stat, faccessat_nonexistant) { + ASSERT_EQ(-1, faccessat(AT_FDCWD, "/blah", F_OK, AT_SYMLINK_NOFOLLOW)); +#if defined(__BIONIC__) + // Android doesn't support AT_SYMLINK_NOFOLLOW + ASSERT_EQ(EINVAL, errno); +#else + ASSERT_EQ(ENOENT, errno); +#endif +}