generic-library/bionic
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Fix "faccessat ignores flags"

Browse Source 
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
This commit is contained in:
Nick Kralevich 2015-02-24 13:40:43 -08:00
parent 2aef607b25
commit 35778253a5
10 changed files with 122 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
libc/Android.mk
View File

@ -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 \

2
libc/SYSCALLS.TXT
View File

@ -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

5
libc/arch-arm/syscalls/faccessat.S → libc/arch-arm/syscalls/___faccessat.S
View File

@ -2,7 +2,7 @@
#include <private/bionic_asm.h>
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

5
libc/arch-arm64/syscalls/faccessat.S → libc/arch-arm64/syscalls/___faccessat.S
View File

@ -2,7 +2,7 @@
#include <private/bionic_asm.h>
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

5
libc/arch-mips/syscalls/faccessat.S → libc/arch-mips/syscalls/___faccessat.S
View File

@ -2,7 +2,7 @@
#include <private/bionic_asm.h>
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

5
libc/arch-mips64/syscalls/faccessat.S → libc/arch-mips64/syscalls/___faccessat.S
View File

@ -2,7 +2,7 @@
#include <private/bionic_asm.h>
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

16
libc/arch-x86/syscalls/faccessat.S → libc/arch-x86/syscalls/___faccessat.S
View File

@ -2,7 +2,7 @@
#include <private/bionic_asm.h>
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

6
libc/arch-x86_64/syscalls/faccessat.S → libc/arch-x86_64/syscalls/___faccessat.S
View File

@ -2,8 +2,7 @@
#include <private/bionic_asm.h>
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

59
libc/bionic/faccessat.cpp Normal file
View File

@ -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 <fcntl.h>
#include <unistd.h>
#include <errno.h>
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);
}

40
tests/sys_stat_test.cpp
View File

@ -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
}