[PATCH 0/2] xap_helper C++ fixes

[PATCH 0/2] xap_helper C++ fixes

[Eric Wong]

Already pushed out since I forgot which VM I was on :x

Eric Wong (2):
  xap_helper: avoid strerror(3) inside signal handler
  xap_helper.h: avoid some off_t vs size_t problems

 lib/PublicInbox/xap_helper.h | 59 ++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

[PATCH 1/2] xap_helper: avoid strerror(3) inside signal handler

[Eric Wong]

It's not async-signal-safe and the glibc implementation uses
malloc via asnprintf.  Practically it's not a problem unless the
kernel OOMs and the write(2) fails to the self-pipe.
---
 lib/PublicInbox/xap_helper.h | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index b6b517d5..1d8437c9 100644
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -980,7 +980,8 @@ static void sigp(int sig) // parent signal handler
 {
 	static const char eagain[] = "signals coming in too fast";
 	static const char bad_sig[] = "BUG: bad sig\n";
-	static const char write_err[] = "BUG: sigp write: ";
+	static const char write_errno[] = "BUG: sigp write (errno)";
+	static const char write_zero[] = "BUG: sigp write wrote zero bytes";
 	char c = 0;
 
 	switch (sig) {
@@ -992,23 +993,17 @@ static void sigp(int sig) // parent signal handler
 		_exit(EXIT_FAILURE);
 	}
 	ssize_t w = write(pipefds[1], &c, 1);
-	if (w == sizeof(c)) return;
-	int e = 0;
-	if (w < 0) {
-		e = errno;
-		if (e == EAGAIN) {
-			write(STDERR_FILENO, eagain, sizeof(eagain) - 1);
-			return;
-		}
+	if (w > 0) return;
+	if (w < 0 && errno == EAGAIN) {
+		write(STDERR_FILENO, eagain, sizeof(eagain) - 1);
+		return;
+	} else if (w == 0) {
+		write(STDERR_FILENO, write_zero, sizeof(write_zero) - 1);
+	} else {
+		// strerror isn't technically async-signal-safe, and
+		// strerrordesc_np+strerrorname_np isn't portable
+		write(STDERR_FILENO, write_errno, sizeof(write_errno) - 1);
 	}
-	struct iovec iov[3];
-	iov[0].iov_base = (void *)write_err;
-	iov[0].iov_len = sizeof(write_err) - 1;
-	iov[1].iov_base = (void *)(e ? strerror(e) : "zero write");
-	iov[1].iov_len = strlen((const char *)iov[1].iov_base);
-	iov[2].iov_base = (void *)"\n";
-	iov[2].iov_len = 1;
-	(void)writev(STDERR_FILENO, iov, MY_ARRAY_SIZE(iov));
 	_exit(EXIT_FAILURE);
 }
 

[PATCH 2/2] xap_helper.h: avoid some off_t vs size_t problems

[Eric Wong]

We'll introduce a helper to cast off_t to size_t consistently
for mmap/munmap/calloc calls which require size_t.  Also, an
extra check for multiplication overflow can be helpful just
in case we end up with a gigantic file roots file.
---
 lib/PublicInbox/xap_helper.h | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index 1d8437c9..5816c24c 100644
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -360,6 +360,13 @@ static void xclose(int fd)
 		EABORT("BUG: close");
 }
 
+static size_t off2size(off_t n)
+{
+	if (n < 0 || (uintmax_t)n > SIZE_MAX)
+		ABORT("off_t out of size_t range: %lld\n", (long long)n);
+	return (size_t)n;
+}
+
 #define CLEANUP_DUMP_ROOTS __attribute__((__cleanup__(dump_roots_ensure)))
 static void dump_roots_ensure(void *ptr)
 {
@@ -367,8 +374,9 @@ static void dump_roots_ensure(void *ptr)
 	if (drt->root2off_fd >= 0)
 		xclose(drt->root2off_fd);
 	hdestroy(); // idempotent
-	if (drt->mm_ptr && munmap(drt->mm_ptr, drt->sb.st_size))
-		EABORT("BUG: munmap(%p, %zu)", drt->mm_ptr, drt->sb.st_size);
+	size_t size = off2size(drt->sb.st_size);
+	if (drt->mm_ptr && munmap(drt->mm_ptr, size))
+		EABORT("BUG: munmap(%p, %zu)", drt->mm_ptr, size);
 	free(drt->entries);
 	fbuf_ensure(&drt->wbuf);
 }
@@ -516,20 +524,18 @@ static bool cmd_dump_roots(struct req *req)
 	// each entry is at least 43 bytes ({OIDHEX}\0{INT}\0),
 	// so /32 overestimates the number of expected entries by
 	// ~%25 (as recommended by Linux hcreate(3) manpage)
-	size_t est = (drt.sb.st_size / 32) + 1; //+1 for "\0" termination
-	if ((uint64_t)drt.sb.st_size > (uint64_t)SIZE_MAX)
-		err(EXIT_FAILURE, "%s size too big (%lld bytes > %zu)",
-			root2off_file, (long long)drt.sb.st_size, SIZE_MAX);
-	drt.mm_ptr = mmap(NULL, drt.sb.st_size, PROT_READ,
+	size_t size = off2size(drt.sb.st_size);
+	size_t est = (size / 32) + 1; //+1 for "\0" termination
+	drt.mm_ptr = mmap(NULL, size, PROT_READ,
 				MAP_PRIVATE, drt.root2off_fd, 0);
 	if (drt.mm_ptr == MAP_FAILED)
-		err(EXIT_FAILURE, "mmap(%zu, %s)",
-			drt.sb.st_size, root2off_file);
-	drt.entries = (char **)calloc(est * 2, sizeof(char *));
+		err(EXIT_FAILURE, "mmap(%zu, %s)", size, root2off_file);
+	size_t asize = est * 2;
+	if (asize < est) ABORT("too many entries: %zu", est);
+	drt.entries = (char **)calloc(asize, sizeof(char *));
 	if (!drt.entries)
 		err(EXIT_FAILURE, "calloc(%zu * 2, %zu)", est, sizeof(char *));
-	size_t tot = split2argv(drt.entries, (char *)drt.mm_ptr,
-				drt.sb.st_size, est * 2);
+	size_t tot = split2argv(drt.entries, (char *)drt.mm_ptr, size, asize);
 	if (tot <= 0) return false; // split2argv already warned on error
 	if (!hcreate(est))
 		err(EXIT_FAILURE, "hcreate(%zu)", est);