Restart failed system calls interrupted with errno of EINTR
In number of places we don't handle properly system calls failures
when the errno is EINTR (i.e., the system call was interrupted
by a signal). In all our use cases, the system calls should be
restarted. The handling of the following system calls (as used in the code)
has been updated/fixed:
poll, send, recv, sendmsg, nanosleep, epoll_wait
read - mostly (e.g., socket-like fds)
write - mostly (e.g., socket-like fds)
select, accept, connect
Bug: 28471477
Bug: 28658141
Change-Id: I03e6f0f67e33876780fb6d02c33eb84547ba8f95
diff --git a/osi/include/osi.h b/osi/include/osi.h
index ed8fc2a..966c617 100644
--- a/osi/include/osi.h
+++ b/osi/include/osi.h
@@ -64,6 +64,9 @@
// Effort is made for this to come from a real random source.
int osi_rand(void);
+// Re-run |fn| system call until the system call doesn't cause EINTR.
+#define OSI_NO_INTR(fn) do {} while ((fn) == -1 && errno == EINTR)
+
#ifdef __cplusplus
}
#endif
diff --git a/osi/include/socket.h b/osi/include/socket.h
index ac49921..b27e681 100644
--- a/osi/include/socket.h
+++ b/osi/include/socket.h
@@ -62,20 +62,24 @@
// block. This function returns a positive integer representing the number
// of bytes copied into |buf| on success, 0 if the socket has disconnected,
// and -1 on error. This function may return a value less than |count| if not
-// enough data is currently available. If this function returns -1, errno will also
-// be set (see recv(2) for possible errno values). If there were no bytes available
-// to be read, this function returns -1 and sets errno to EWOULDBLOCK. Neither
-// |socket| nor |buf| may be NULL.
+// enough data is currently available. If this function returns -1, errno will
+// also be set (see recv(2) for possible errno values). However, if the reading
+// system call was interrupted with errno of EINTR, the read operation is
+// restarted internally without propagating EINTR back to the caller. If there
+// were no bytes available to be read, this function returns -1 and sets errno
+// to EWOULDBLOCK. Neither |socket| nor |buf| may be NULL.
ssize_t socket_read(const socket_t *socket, void *buf, size_t count);
// Writes up to |count| bytes from |buf| into |socket|. This function will not
// block. Returns a positive integer representing the number of bytes written
-// to |socket| on success, 0 if the socket has disconnected, and -1 on error. This
-// function may return a value less than |count| if writing more bytes would result
-// in blocking. If this function returns -1, errno will also be set (see send(2) for
-// possible errno values). If no bytes could be written without blocking, this
-// function will return -1 and set errno to EWOULDBLOCK. Neither |socket| nor |buf|
-// may be NULL.
+// to |socket| on success, 0 if the socket has disconnected, and -1 on error.
+// This function may return a value less than |count| if writing more bytes
+// would result in blocking. If this function returns -1, errno will also be
+// set (see send(2) for possible errno values). However, if the writing system
+// call was interrupted with errno of EINTR, the write operation is restarted
+// internally without propagating EINTR back to the caller. If no bytes could
+// be written without blocking, this function will return -1 and set errno to
+// EWOULDBLOCK. Neither |socket| nor |buf| may be NULL.
ssize_t socket_write(const socket_t *socket, const void *buf, size_t count);
// This function performs the same write operation as |socket_write| and also
diff --git a/osi/src/eager_reader.c b/osi/src/eager_reader.c
index 044ee81..07a944c 100644
--- a/osi/src/eager_reader.c
+++ b/osi/src/eager_reader.c
@@ -215,15 +215,23 @@
assert(reader != NULL);
fd_set read_fds;
- FD_ZERO(&read_fds);
- FD_SET(reader->bytes_available_fd, &read_fds);
- // Immediate timeout
- struct timeval timeout;
- timeout.tv_sec = 0;
- timeout.tv_usec = 0;
+ for (;;) {
+ FD_ZERO(&read_fds);
+ FD_SET(reader->bytes_available_fd, &read_fds);
- select(reader->bytes_available_fd + 1, &read_fds, NULL, NULL, &timeout);
+ // Immediate timeout
+ struct timeval timeout;
+ timeout.tv_sec = 0;
+ timeout.tv_usec = 0;
+
+ int ret = select(reader->bytes_available_fd + 1, &read_fds, NULL, NULL,
+ &timeout);
+ if (ret == -1 && errno == EINTR)
+ continue;
+ break;
+ }
+
return FD_ISSET(reader->bytes_available_fd, &read_fds);
}
@@ -239,7 +247,9 @@
buffer->length = 0;
buffer->offset = 0;
- int bytes_read = read(reader->inbound_fd, buffer->data, reader->buffer_size);
+ ssize_t bytes_read;
+ OSI_NO_INTR(bytes_read = read(reader->inbound_fd, buffer->data,
+ reader->buffer_size));
if (bytes_read > 0) {
// Save the data for later
buffer->length = bytes_read;
diff --git a/osi/src/metrics.cc b/osi/src/metrics.cc
index a99200b..064f99e 100644
--- a/osi/src/metrics.cc
+++ b/osi/src/metrics.cc
@@ -196,7 +196,9 @@
std::string protoBase64;
base::Base64Encode(serialized, &protoBase64);
- if (write(fd, protoBase64.c_str(), protoBase64.size()) == -1) {
+ ssize_t ret;
+ OSI_NO_INTR(ret = write(fd, protoBase64.c_str(), protoBase64.size()));
+ if (ret == -1) {
LOG_ERROR(LOG_TAG, "%s: error writing to dumpsys fd: %s (%d)", __func__,
strerror(errno), errno);
}
@@ -215,9 +217,10 @@
}
log_lock.unlock();
- if (write(fd, pretty_output.c_str(), pretty_output.size()) == -1) {
+ ssize_t ret;
+ OSI_NO_INTR(ret = write(fd, pretty_output.c_str(), pretty_output.size()));
+ if (ret == -1) {
LOG_ERROR(LOG_TAG, "%s: error writing to dumpsys fd: %s (%d)", __func__,
strerror(errno), errno);
}
-
}
diff --git a/osi/src/reactor.c b/osi/src/reactor.c
index 7bab5e4..7388d52 100644
--- a/osi/src/reactor.c
+++ b/osi/src/reactor.c
@@ -237,10 +237,7 @@
pthread_mutex_unlock(&reactor->list_lock);
int ret;
- do {
- ret = epoll_wait(reactor->epoll_fd, events, MAX_EVENTS, -1);
- } while (ret == -1 && errno == EINTR);
-
+ OSI_NO_INTR(ret = epoll_wait(reactor->epoll_fd, events, MAX_EVENTS, -1));
if (ret == -1) {
LOG_ERROR(LOG_TAG, "%s error in epoll_wait: %s", __func__, strerror(errno));
reactor->is_running = false;
diff --git a/osi/src/socket.c b/osi/src/socket.c
index 80813f5..4ebb3b8 100644
--- a/osi/src/socket.c
+++ b/osi/src/socket.c
@@ -113,7 +113,8 @@
socket_t *socket_accept(const socket_t *socket) {
assert(socket != NULL);
- int fd = accept(socket->fd, NULL, NULL);
+ int fd;
+ OSI_NO_INTR(fd = accept(socket->fd, NULL, NULL));
if (fd == INVALID_FD) {
LOG_ERROR(LOG_TAG, "%s unable to accept socket: %s", __func__, strerror(errno));
return NULL;
@@ -129,14 +130,20 @@
assert(socket != NULL);
assert(buf != NULL);
- return recv(socket->fd, buf, count, MSG_DONTWAIT);
+ ssize_t ret;
+ OSI_NO_INTR(ret = recv(socket->fd, buf, count, MSG_DONTWAIT));
+
+ return ret;
}
ssize_t socket_write(const socket_t *socket, const void *buf, size_t count) {
assert(socket != NULL);
assert(buf != NULL);
- return send(socket->fd, buf, count, MSG_DONTWAIT);
+ ssize_t ret;
+ OSI_NO_INTR(ret = send(socket->fd, buf, count, MSG_DONTWAIT));
+
+ return ret;
}
ssize_t socket_write_and_transfer_fd(const socket_t *socket, const void *buf, size_t count, int fd) {
@@ -166,7 +173,9 @@
header->cmsg_len = CMSG_LEN(sizeof(int));
*(int *)CMSG_DATA(header) = fd;
- ssize_t ret = sendmsg(socket->fd, &msg, MSG_DONTWAIT);
+ ssize_t ret;
+ OSI_NO_INTR(ret = sendmsg(socket->fd, &msg, MSG_DONTWAIT));
+
close(fd);
return ret;
}
diff --git a/osi/src/socket_utils/socket_local_client.c b/osi/src/socket_utils/socket_local_client.c
index ccfe743..73a828c 100644
--- a/osi/src/socket_utils/socket_local_client.c
+++ b/osi/src/socket_utils/socket_local_client.c
@@ -121,7 +121,8 @@
goto error;
}
- if (connect(fd, (struct sockaddr *)&addr, alen) < 0) {
+ OSI_NO_INTR(err = connect(fd, (struct sockaddr *)&addr, alen));
+ if (err < 0) {
goto error;
}