Simplify close(2) EINTR handling.
This doesn't affect code like Chrome that correctly ignores EINTR on
close, makes code that tries TEMP_FAILURE_RETRY work (where before it might
have closed a different fd and appeared to succeed, or had a bogus EBADF),
and makes "goto fail" code work (instead of mistakenly assuming that EINTR
means that the close failed).
Who loses? Anyone actively trying to detect that they caught a signal while
in close(2). I don't think those people exist, and I think they have better
alternatives available.
Bug: https://code.google.com/p/chromium/issues/detail?id=269623
Bug: http://b/20501816
Change-Id: I11e2f66532fe5d1b0082b2433212e24bdda8219b
diff --git a/libc/Android.mk b/libc/Android.mk
index 4a199e7..5ed9ae4 100644
--- a/libc/Android.mk
+++ b/libc/Android.mk
@@ -109,6 +109,7 @@
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|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
similarity index 84%
rename from libc/arch-arm/syscalls/close.S
rename to 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 @@
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
similarity index 82%
rename from libc/arch-arm64/syscalls/close.S
rename to 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 @@
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
similarity index 85%
rename from libc/arch-mips/syscalls/close.S
rename to 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 @@
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
similarity index 88%
rename from libc/arch-mips64/syscalls/close.S
rename to 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 @@
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
similarity index 89%
rename from libc/arch-x86/syscalls/close.S
rename to 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 @@
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
similarity index 84%
rename from libc/arch-x86_64/syscalls/close.S
rename to 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 @@
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;
+}