wire_sync: tolerate a non-blocking fd when reading/writing a message

On macOS under load, subdaemons intermittently die:

  connectd: **BROKEN** STATUS_FAIL_HSM_IO: No hsmd ECDH response
  channeld: exits 0 after WIRE_HSMD_GET_PER_COMMITMENT_POINT

Both are a synchronous wire_sync_read() returning NULL on a fresh
connection.  The HSM fd in a subdaemon is one end of a socketpair created
by hsmd (whose io loop sets O_NONBLOCK on the other end) and passed
hsmd -> lightningd -> subdaemon via SCM_RIGHTS.  On macOS the O_NONBLOCK
flag follows the shared open file description across that chain, so the
subdaemon's fd can be non-blocking, and read()/write() return EAGAIN
before hsmd's (fast) reply has landed.  Linux keeps the descriptions
independent, which is why this never reproduces there.

The previous attempts to force the fd blocking from the subdaemon
(io_fd_block in ecdh_hsmd_setup) cannot win, because hsmd owns the other
end of the same open file description and keeps it non-blocking.

Fix at the shared choke point instead: make wire_sync_read() and
wire_sync_write() tolerant of O_NONBLOCK by polling on EAGAIN and
resuming, preserving any partial read.  "Sync" then really means "read or
write a complete message", regardless of the fd's blocking state.  This
covers connectd's ecdh(), channeld's hsm_req(), and every other subdaemon
that does synchronous HSM I/O (openingd, closingd, onchaind).

Also drop the now-unneeded io_fd_block() toggling from ecdh_hsmd_setup(),
and include errno in the HSM I/O failure messages so a recurrence is
diagnosable from the daemon log.

Changelog-Fixed: connectd: fix intermittent "No hsmd ECDH response" crash on macOS under load (issue #9060).

common/ecdh_hsmd.c

@@ -4,6 +4,7 @@
 #include <common/ecdh.h>
 #include <common/ecdh_hsmd.h>
 #include <common/utils.h>
+#include <errno.h>
 #include <hsmd/hsmd_wiregen.h>
 #include <wire/wire_sync.h>
 
@@ -13,16 +14,22 @@ static void (*stashed_failed)(enum status_failreason, const char *fmt, ...);
 void ecdh(const struct pubkey *point, struct secret *ss)
 {
 	const u8 *msg = towire_hsmd_ecdh_req(NULL, point);
+	u8 *resp;
 
 	assert(stashed_hsm_fd >= 0);
 	assert(stashed_failed != NULL);
 
+	/* wire_sync_read/write tolerate a non-blocking fd (the HSM socketpair
+	 * can be O_NONBLOCK on macOS), so no blocking toggling is needed here.
+	 * Report errno so a failure is diagnosable from the daemon log. */
 	if (!wire_sync_write(stashed_hsm_fd, take(msg)))
-		stashed_failed(STATUS_FAIL_HSM_IO, "Write ECDH to hsmd failed");
+		stashed_failed(STATUS_FAIL_HSM_IO, "Write ECDH to hsmd failed: %s",
+			       strerror(errno));
 
-	msg = wire_sync_read(tmpctx, stashed_hsm_fd);
-	if (!msg)
-		stashed_failed(STATUS_FAIL_HSM_IO, "No hsmd ECDH response");
+	resp = wire_sync_read(tmpctx, stashed_hsm_fd);
+	if (!resp)
+		stashed_failed(STATUS_FAIL_HSM_IO, "No hsmd ECDH response: %s",
+			       strerror(errno));
 
 	if (!fromwire_hsmd_ecdh_resp(msg, ss))
 		stashed_failed(STATUS_FAIL_HSM_IO, "Invalid hsmd ECDH response");
@@ -34,6 +41,4 @@ void ecdh_hsmd_setup(int hsm_fd,
 {
 	stashed_hsm_fd = hsm_fd;
 	stashed_failed = failed;
-	/* Like read_fds in subd.c: don't trust sender's O_NONBLOCK state (issue #9060). */
-	io_fd_block(hsm_fd, true);
 }

wire/wire_sync.c

@@ -1,19 +1,83 @@
 #include "config.h"
 #include <assert.h>
-#include <ccan/read_write_all/read_write_all.h>
 #include <common/utils.h>
 #include <errno.h>
+#include <poll.h>
 #include <wire/wire_io.h>
 #include <wire/wire_sync.h>
 
+/* Wait until fd is ready (readable or writable), tolerating EINTR. */
+static bool wait_fd(int fd, short events)
+{
+	int r;
+
+	do {
+		r = poll(&(struct pollfd){.fd = fd, .events = events}, 1, -1);
+	} while (r < 0 && errno == EINTR);
+
+	return r > 0;
+}
+
+/* Like read_all, but tolerates an fd with O_NONBLOCK set: on EAGAIN we poll
+ * for readability and resume, preserving any partial read.  Subdaemon HSM fds
+ * can be O_NONBLOCK on macOS (the flag follows the shared open file
+ * description as the socketpair is sent via SCM_RIGHTS), so without this we
+ * would spuriously fail with EAGAIN. */
+static bool read_all_tolerant(int fd, void *buf, size_t size)
+{
+	while (size) {
+		ssize_t done = read(fd, buf, size);
+		if (done < 0) {
+			if (errno == EINTR)
+				continue;
+			if (errno == EAGAIN || errno == EWOULDBLOCK) {
+				if (!wait_fd(fd, POLLIN))
+					return false;
+				continue;
+			}
+			return false;
+		}
+		if (done == 0)
+			return false;
+		buf = (char *)buf + done;
+		size -= done;
+	}
+
+	return true;
+}
+
+/* Mirror of read_all_tolerant for writes. */
+static bool write_all_tolerant(int fd, const void *buf, size_t size)
+{
+	while (size) {
+		ssize_t done = write(fd, buf, size);
+		if (done < 0) {
+			if (errno == EINTR)
+				continue;
+			if (errno == EAGAIN || errno == EWOULDBLOCK) {
+				if (!wait_fd(fd, POLLOUT))
+					return false;
+				continue;
+			}
+			return false;
+		}
+		if (done == 0)
+			return false;
+		buf = (const char *)buf + done;
+		size -= done;
+	}
+
+	return true;
+}
+
 bool wire_sync_write(int fd, const void *msg TAKES)
 {
 	wire_len_t hdr = cpu_to_wirelen(tal_bytelen(msg));
 	bool ret;
 
 	assert(tal_bytelen(msg) < WIRE_LEN_LIMIT);
-	ret = write_all(fd, &hdr, sizeof(hdr))
-		&& write_all(fd, msg, tal_count(msg));
+	ret = write_all_tolerant(fd, &hdr, sizeof(hdr))
+		&& write_all_tolerant(fd, msg, tal_count(msg));
 
 	tal_free_if_taken(msg);
 	return ret;
@@ -24,14 +88,14 @@ u8 *wire_sync_read(const tal_t *ctx, int fd)
 	wire_len_t len;
 	u8 *msg;
 
-	if (!read_all(fd, &len, sizeof(len)))
+	if (!read_all_tolerant(fd, &len, sizeof(len)))
 		return NULL;
 	if (wirelen_to_cpu(len) >= WIRE_LEN_LIMIT) {
 		errno = E2BIG;
 		return NULL;
 	}
 	msg = tal_arr(ctx, u8, wirelen_to_cpu(len));
-	if (!read_all(fd, msg, wirelen_to_cpu(len)))
+	if (!read_all_tolerant(fd, msg, wirelen_to_cpu(len)))
 		return tal_free(msg);
 	return msg;
 }