Skip to content

Commit a904af9

Browse files
Improve EINTR handling for close in zero_copy_stream_impl.h
When `close(fd)` returns an error and `errno == EINTR`, it depends on the exact operating system used whether `fd` has been closed or not. This is bad for us: if we *do* retry `close(fd)` but `fd` *was closed*, then it might be that `fd` has already been given out again, and we will close a file descriptor which is still in use. If we *do not* retry `close(fd)` and `fd` was *not closed*, we will leak `fd`. It turns out that this is a rather complicated issue. I will try to first give an overview, then explain what the new code does. ### Overview [POSIX-2016](https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/functions/close.html) and earlier explicitly said that it is unspecified whether `fd` is closed in case it returns -1 with `errno == EINTR` (the text uses `filedes` instead of `fd`): > If `close()` is interrupted by a signal that is to be caught, it shall return > -1 with `errno` set to `[EINTR]` and the state of `fildes` is unspecified. As one might expect given this, this means that everything is a mess. Some operating systems left `fd` open, others closed it. I could figure out the following: * [Linux closes `fd`](https://man7.org/linux/man-pages/man2/close.2.html). * [HP-UX keeps `fd` open](https://www.unix.com/man_page/hpux/2/close) * MacOS keeps it open (at least this is implied by https://crbug.com/269623#comment1). A good overview of the situation before POSIX.1-2024 is given in https://crbug.com/269623#comment1. Then came [POSIX.1-2024](https://pubs.opengroup.org/onlinepubs/9799919799/functions/close.html). This explicitly specifies that if close returns EINTR it *must* keep `fd` open: > If `close()` \[with argument `filedes`\] is interrupted by a signal that is to be caught, then it is unspecified whether it returns -1 with `errno` set to `[EINTR]` and `fildes` remaining open, or returns -1 with `errno` set to `[EINPROGRESS]` and `fildes` being closed, or returns 0 to indicate successful completion. This actually sounds good for the code before this CL: it specifies that the code here is correct! Unfortunately for this code, it seems it doesn't matter to us what POSIX specifes: the Linux maintainers very clearly do not intend to adhere to this. [`man 2 close`](https://man7.org/linux/man-pages/man2/close.2.html) explicitly states in the end that Linux is non-conforming and will not change. The [corresponding discussion](https://lore.kernel.org/all/fskxqmcszalz6dmoak6de4c7bxt4juvc5zrpboae4dqw4y6aih@lskezjrbnsws/) from 2025-05-15 in the mailing list also explicitly states that it is possible that Linux returns EINTR and has `fd` closed. ### Proposal Given all this, we hence propose the following: * if POSIX_CLOSE_RESTART is defined, we assume we have a POSIX.1-2024 compliant system. In this case, we simply use [posix_close](https://pubs.opengroup.org/onlinepubs/9799919799/functions/close.html#tag_17_81) which allows us to specify the behavior to what we want. * Otherwise, we fallback to simply calling close once. In the end, this is the safer option than retrying, since this will only leak `fd` if something goes wrong. If we call close twice when we should not, we risk closing `fd` while it is still in use. ### More references: Chromium: [scoped_file](https://source.chromium.org/chromium/chromium/src/+/main:base/files/scoped_file.cc;drc=4bfcab51c9e72a7bdc1abb6c436a8e6b93dc1689) and [hack for MacOS](https://source.chromium.org/chromium/chromium/src/+/main:base/mac/close_nocancel.cc;l=51;drc=068b7e346d819075983f831a853bdf1da287973c) Riegeli: [fd_handle](https://github.com/google/riegeli/blob/e1b2fedf24735a1221f44cd01a6d0d79a522d5aa/riegeli/bytes/fd_handle.h#L367-L380) PiperOrigin-RevId: 861056844
1 parent a0b7794 commit a904af9

1 file changed

Lines changed: 19 additions & 15 deletions

File tree

src/google/protobuf/io/zero_copy_stream_impl.cc

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@
99
// Based on original Protocol Buffers design by
1010
// Sanjay Ghemawat, Jeff Dean, and others.
1111

12+
// We request posix_close if available. See the comment on "robust_close".
13+
#define _POSIX_C_SOURCE 202405L
14+
1215
#ifndef _MSC_VER
1316
#include <fcntl.h>
1417
#include <sys/stat.h>
@@ -47,13 +50,20 @@ using google::protobuf::io::win32::write;
4750

4851
namespace {
4952

50-
// EINTR sucks.
51-
int close_no_eintr(int fd) {
52-
int result;
53-
do {
54-
result = close(fd);
55-
} while (result < 0 && errno == EINTR);
56-
return result;
53+
// If close(fd) returns an error, there is really nothing special to do --
54+
// except on *some* systems in case `errno == EINTR`. Unfortunately, that case
55+
// is also a huge mess and the question whether fd was closed in this case
56+
// depends on the system. Linux *did* close `fd` in this case. POSIX.1-2024
57+
// introduced posix_close (and did other changes) which helps fix this mess.
58+
// The POSIX.1-2024 documentation for unistd.h specifies that
59+
// POSIX_CLOSE_RESTART needs to be provided (and the value defines the behavior
60+
// of posix_close). We use it to decide if we can use posix_close.
61+
int robust_close(int fd) {
62+
#if defined(POSIX_CLOSE_RESTART)
63+
return posix_close(fd, 0);
64+
#else
65+
return close(fd);
66+
#endif
5767
}
5868

5969
} // namespace
@@ -101,10 +111,7 @@ bool FileInputStream::CopyingFileInputStream::Close() {
101111
ABSL_CHECK(!is_closed_);
102112

103113
is_closed_ = true;
104-
if (close_no_eintr(file_) != 0) {
105-
// The docs on close() do not specify whether a file descriptor is still
106-
// open after close() fails with EIO. However, the glibc source code
107-
// seems to indicate that it is not.
114+
if (robust_close(file_) != 0) {
108115
errno_ = errno;
109116
return false;
110117
}
@@ -178,10 +185,7 @@ bool FileOutputStream::CopyingFileOutputStream::Close() {
178185
ABSL_CHECK(!is_closed_);
179186

180187
is_closed_ = true;
181-
if (close_no_eintr(file_) != 0) {
182-
// The docs on close() do not specify whether a file descriptor is still
183-
// open after close() fails with EIO. However, the glibc source code
184-
// seems to indicate that it is not.
188+
if (robust_close(file_) != 0) {
185189
errno_ = errno;
186190
return false;
187191
}

0 commit comments

Comments
 (0)