Merge "Simplify close(2) EINTR handling."

9 files changed, 76 insertions, 13 deletions

diff --git a/libc/Android.mk b/libc/Android.mk
index 8ed969f..5c1a1a5 100644
--- a/libc/Android.mk
+++ b/libc/Android.mk
@@ -110,6 +110,7 @@ libc_bionic_ndk_src_files := \
     bionic/clock_getcpuclockid.cpp \
     bionic/clock_nanosleep.cpp \
     bionic/clone.cpp \
+    bionic/close.cpp \
     bionic/__cmsg_nxthdr.cpp \
     bionic/connect.cpp \
     bionic/ctype.cpp \
diff --git a/libc/SYSCALLS.TXT b/libc/SYSCALLS.TXT
index b91f5bf..33e30eb 100644
--- a/libc/SYSCALLS.TXT
+++ b/libc/SYSCALLS.TXT
@@ -95,7 +95,7 @@ ssize_t     pread64(int, void*, size_t, off64_t) arm,mips,x86
 ssize_t     pread64|pread(int, void*, size_t, off_t) arm64,mips64,x86_64
 ssize_t     pwrite64(int, void*, size_t, off64_t) arm,mips,x86
 ssize_t     pwrite64|pwrite(int, void*, size_t, off_t) arm64,mips64,x86_64
-int         close(int)                      all
+int         ___close:close(int)  all
 pid_t       __getpid:getpid()  all
 int         munmap(void*, size_t)  all
 void*       mremap(void*, size_t, size_t, unsigned long)  all
diff --git a/libc/arch-arm/syscalls/close.S b/libc/arch-arm/syscalls/___close.S
index ec05445..db8a230 100644
--- a/libc/arch-arm/syscalls/close.S
+++ b/libc/arch-arm/syscalls/___close.S
@@ -2,7 +2,7 @@
 
 #include <private/bionic_asm.h>
 
-ENTRY(close)
+ENTRY(___close)
     mov     ip, r7
     ldr     r7, =__NR_close
     swi     #0
@@ -11,4 +11,5 @@ ENTRY(close)
     bxls    lr
     neg     r0, r0
     b       __set_errno_internal
-END(close)
+END(___close)
+.hidden ___close
diff --git a/libc/arch-arm64/syscalls/close.S b/libc/arch-arm64/syscalls/___close.S
index 3624581..8fb8361 100644
--- a/libc/arch-arm64/syscalls/close.S
+++ b/libc/arch-arm64/syscalls/___close.S
@@ -2,7 +2,7 @@
 
 #include <private/bionic_asm.h>
 
-ENTRY(close)
+ENTRY(___close)
     mov     x8, __NR_close
     svc     #0
 
@@ -11,4 +11,5 @@ ENTRY(close)
     b.hi    __set_errno_internal
 
     ret
-END(close)
+END(___close)
+.hidden ___close
diff --git a/libc/arch-mips/syscalls/close.S b/libc/arch-mips/syscalls/___close.S
index 231f497..356cfd6 100644
--- a/libc/arch-mips/syscalls/close.S
+++ b/libc/arch-mips/syscalls/___close.S
@@ -2,7 +2,7 @@
 
 #include <private/bionic_asm.h>
 
-ENTRY(close)
+ENTRY(___close)
     .set noreorder
     .cpload t9
     li v0, __NR_close
@@ -16,4 +16,5 @@ ENTRY(close)
     j t9
     nop
     .set reorder
-END(close)
+END(___close)
+.hidden ___close
diff --git a/libc/arch-mips64/syscalls/close.S b/libc/arch-mips64/syscalls/___close.S
index 5e237dd..f1ce708 100644
--- a/libc/arch-mips64/syscalls/close.S
+++ b/libc/arch-mips64/syscalls/___close.S
@@ -2,7 +2,7 @@
 
 #include <private/bionic_asm.h>
 
-ENTRY(close)
+ENTRY(___close)
     .set push
     .set noreorder
     li v0, __NR_close
@@ -22,4 +22,5 @@ ENTRY(close)
     j t9
     move ra, t0
     .set pop
-END(close)
+END(___close)
+.hidden ___close
diff --git a/libc/arch-x86/syscalls/close.S b/libc/arch-x86/syscalls/___close.S
index f6cce62..796944b 100644
--- a/libc/arch-x86/syscalls/close.S
+++ b/libc/arch-x86/syscalls/___close.S
@@ -2,7 +2,7 @@
 
 #include <private/bionic_asm.h>
 
-ENTRY(close)
+ENTRY(___close)
     pushl   %ebx
     .cfi_def_cfa_offset 8
     .cfi_rel_offset ebx, 0
@@ -18,4 +18,5 @@ ENTRY(close)
 1:
     popl    %ebx
     ret
-END(close)
+END(___close)
+.hidden ___close
diff --git a/libc/arch-x86_64/syscalls/close.S b/libc/arch-x86_64/syscalls/___close.S
index 8a7ada1..8607f05 100644
--- a/libc/arch-x86_64/syscalls/close.S
+++ b/libc/arch-x86_64/syscalls/___close.S
@@ -2,7 +2,7 @@
 
 #include <private/bionic_asm.h>
 
-ENTRY(close)
+ENTRY(___close)
     movl    $__NR_close, %eax
     syscall
     cmpq    $-MAX_ERRNO, %rax
@@ -12,4 +12,5 @@ ENTRY(close)
     call    __set_errno_internal
 1:
     ret
-END(close)
+END(___close)
+.hidden ___close
diff --git a/libc/bionic/close.cpp b/libc/bionic/close.cpp
new file mode 100644
index 0000000..18225f0
--- /dev/null
+++ b/libc/bionic/close.cpp
@@ -0,0 +1,56 @@
+/*
+ * 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 <errno.h>
+#include <unistd.h>
+
+extern "C" int ___close(int);
+
+int close(int fd) {
+  int rc = ___close(fd);
+  if (rc == -1 && errno == EINTR) {
+    // POSIX says that if close returns with EINTR, the fd must not be closed.
+    // Linus disagrees: http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
+    // The future POSIX solution is posix_close (http://austingroupbugs.net/view.php?id=529),
+    // with the state after EINTR being undefined, and EINPROGRESS for the case where close
+    // was interrupted by a signal but the file descriptor was actually closed.
+    // My concern with that future behavior is that it breaks existing code that assumes
+    // that close only returns -1 if it failed. Unlike other system calls, I have real
+    // difficulty even imagining a caller that would need to know that close was interrupted
+    // but succeeded. So returning EINTR is wrong (because Linux always closes) and EINPROGRESS
+    // is harmful because callers need to be rewritten to understand that EINPROGRESS isn't
+    // actually a failure, but will be reported as one.
+
+    // We don't restore errno because that would incur a cost (the TLS read) for every caller.
+    // Since callers don't know ahead of time whether close will legitimately fail, they need
+    // to have stashed the old errno value anyway if they plan on using it afterwards, so
+    // us clobbering errno here doesn't change anything in that respect.
+    return 0;
+  }
+  return rc;
+}