[PATCH 0/5] Reintroduce writev(3p)

[PATCH 0/5] Reintroduce writev(3p)

[Patrick Steinhardt]

Hi,

this patch series reintroduces the writev(3p) wrapper. This wrapper was
originally introduced as part of Git 2.54 [1], but was ejected due to
issues on NonStop [2].

This patch series here revives the effort with a couple of fixes on top:

  - It picks Dscho's fix for CMake [3].

  - It picks a fix for NonStop [4] and polishes it a bit.

  - It adapts one more site to demonstrate that its usefulness is not
    limited to a single callsite, only.

Furthermore, I have included benchmarks now that demonstrate the
benefits to make this series a bit more appealing. Ultimately, I'd be
fine if we say we rather don't want to go this way though. I merely
wanted to tie some loose ends that I left dangling.

That, and it's nice to not work on pluggable object databases once in a
while.

Thanks!

Patrick

[1]: <20260227-pks-upload-pack-write-contention-v1-0-7166fe255704@pks.im>
[2]: <028901dcc859$d2419470$76c4bd50$@nexbridge.com>
[3]: <pull.2078.git.1775206502134.gitgitgadget@gmail.com>
[4]: <20260409-b4-pks-writev-max-io-size-v1-1-81730e8f35df@pks.im>

---
Patrick Steinhardt (5):
      compat/posix: introduce writev(3p) wrapper
      wrapper: introduce writev(3p) wrappers
      wrapper: properly handle MAX_IO_SIZE in writev(3p)
      sideband: use writev(3p) to send pktlines
      fast-import: use writev(3p) to send cat-blob responses

 Makefile                            |  4 ++
 builtin/fast-import.c               | 18 +++++++--
 compat/posix.h                      | 14 +++++++
 compat/writev.c                     | 44 +++++++++++++++++++++
 config.mak.uname                    |  2 +
 contrib/buildsystems/CMakeLists.txt |  6 ++-
 meson.build                         |  1 +
 sideband.c                          | 14 +++++--
 wrapper.c                           | 78 +++++++++++++++++++++++++++++++++++++
 wrapper.h                           | 10 +++++
 write-or-die.c                      |  8 ++++
 write-or-die.h                      |  1 +
 12 files changed, 193 insertions(+), 7 deletions(-)


---
base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
change-id: 20260714-pks-reintroduce-writev-2d8f7e52eee9

[PATCH 1/5] compat/posix: introduce writev(3p) wrapper

[Patrick Steinhardt]

In a subsequent commit we're going to add the first caller to
writev(3p). Introduce a compatibility wrapper for this syscall that we
can use on systems that don't have this syscall.

The syscall exists on modern Unixes like Linux and macOS, and seemingly
even for NonStop according to [1]. It doesn't seem to exist on Windows
though.

[1]: http://nonstoptools.com/manuals/OSS-SystemCalls.pdf
[2]: https://www.gnu.org/software/gnulib/manual/html_node/writev.html

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile                            |  4 ++++
 compat/posix.h                      | 14 ++++++++++++
 compat/writev.c                     | 44 +++++++++++++++++++++++++++++++++++++
 config.mak.uname                    |  2 ++
 contrib/buildsystems/CMakeLists.txt |  6 ++++-
 meson.build                         |  1 +
 6 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1f3f099f5c..eda5ecc5b4 100644
--- a/Makefile
+++ b/Makefile
@@ -2033,6 +2033,10 @@ ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
 endif
+ifdef NO_WRITEV
+	COMPAT_CFLAGS += -DNO_WRITEV
+	COMPAT_OBJS += compat/writev.o
+endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
 endif
diff --git a/compat/posix.h b/compat/posix.h
index e2e794cad7..71cc731620 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -148,6 +148,9 @@
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <sys/statvfs.h>
+#ifndef NO_WRITEV
+#include <sys/uio.h>
+#endif
 #include <termios.h>
 #ifndef NO_SYS_SELECT_H
 #include <sys/select.h>
@@ -334,6 +337,17 @@ int git_lstat(const char *, struct stat *);
 ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
 #endif
 
+#ifdef NO_WRITEV
+#define writev git_writev
+#define iovec git_iovec
+struct git_iovec {
+	void *iov_base;
+	size_t iov_len;
+};
+
+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt);
+#endif
+
 #ifdef NO_SETENV
 #define setenv gitsetenv
 int gitsetenv(const char *, const char *, int);
diff --git a/compat/writev.c b/compat/writev.c
new file mode 100644
index 0000000000..ab2e223634
--- /dev/null
+++ b/compat/writev.c
@@ -0,0 +1,44 @@
+#include "../git-compat-util.h"
+#include "../wrapper.h"
+
+ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
+{
+	size_t total_written = 0;
+	size_t sum = 0;
+
+	/*
+	 * According to writev(3p), the syscall shall error with EINVAL in case
+	 * the sum of `iov_len` overflows `ssize_t`.
+	 */
+	for (int i = 0; i < iovcnt; i++) {
+		if (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||
+		    iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {
+			errno = EINVAL;
+			return -1;
+		}
+
+		sum += iov[i].iov_len;
+	}
+
+	for (int i = 0; i < iovcnt; i++) {
+		const char *bytes = iov[i].iov_base;
+		size_t iovec_written = 0;
+
+		while (iovec_written < iov[i].iov_len) {
+			ssize_t bytes_written = xwrite(fd, bytes + iovec_written,
+						       iov[i].iov_len - iovec_written);
+			if (bytes_written < 0) {
+				if (total_written)
+					goto out;
+				return bytes_written;
+			}
+			if (!bytes_written)
+				goto out;
+			iovec_written += bytes_written;
+			total_written += bytes_written;
+		}
+	}
+
+out:
+	return (ssize_t) total_written;
+}
diff --git a/config.mak.uname b/config.mak.uname
index 9ebd240378..95ef6e64dc 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -483,6 +483,7 @@ ifeq ($(uname_S),Windows)
 	SANE_TOOL_PATH ?= $(msvc_bin_dir_msys)
 	HAVE_ALLOCA_H = YesPlease
 	NO_PREAD = YesPlease
+	NO_WRITEV = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
@@ -697,6 +698,7 @@ ifeq ($(uname_S),MINGW)
 	pathsep = ;
 	HAVE_ALLOCA_H = YesPlease
 	NO_PREAD = YesPlease
+	NO_WRITEV = YesPlease
 	NEEDS_CRYPTO_WITH_SSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_POLL = YesPlease
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index a57c4b464f..8f56203f34 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -378,7 +378,7 @@ endif()
 #function checks
 set(function_checks
 	strcasestr memmem strlcpy strtoimax strtoumax strtoull
-	setenv mkdtemp poll pread memmem)
+	setenv mkdtemp poll pread memmem writev)
 
 #unsetenv,hstrerror are incompatible with windows build
 if(NOT WIN32)
@@ -423,6 +423,10 @@ if(NOT HAVE_MEMMEM)
 	list(APPEND compat_SOURCES compat/memmem.c)
 endif()
 
+if(NOT HAVE_WRITEV)
+	list(APPEND compat_SOURCES compat/writev.c)
+endif()
+
 if(NOT WIN32)
 	if(NOT HAVE_UNSETENV)
 		list(APPEND compat_SOURCES compat/unsetenv.c)
diff --git a/meson.build b/meson.build
index ca235801cf..613828ff25 100644
--- a/meson.build
+++ b/meson.build
@@ -1446,6 +1446,7 @@ checkfuncs = {
   'initgroups' : [],
   'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
   'pread' : ['pread.c'],
+  'writev' : ['writev.c'],
 }
 
 if host_machine.system() == 'windows'

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH 2/5] wrapper: introduce writev(3p) wrappers

[Patrick Steinhardt]

In the preceding commit we have added a compatibility wrapper for the
writev(3p) syscall. Introduce some generic wrappers for this function
that we nowadays take for granted in the Git codebase.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 wrapper.c      | 41 +++++++++++++++++++++++++++++++++++++++++
 wrapper.h      |  9 +++++++++
 write-or-die.c |  8 ++++++++
 write-or-die.h |  1 +
 4 files changed, 59 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index 16f5a63fbb..be8fa575e6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -323,6 +323,47 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
 	return total;
 }
 
+ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
+{
+	ssize_t total_written = 0;
+
+	while (iovcnt) {
+		ssize_t bytes_written = writev(fd, iov, iovcnt);
+		if (bytes_written < 0) {
+			if (errno == EINTR || errno == EAGAIN)
+				continue;
+			return -1;
+		}
+		if (!bytes_written) {
+			errno = ENOSPC;
+			return -1;
+		}
+
+		total_written += bytes_written;
+
+		/*
+		 * We first need to discard any iovec entities that have been
+		 * fully written.
+		 */
+		while (iovcnt && (size_t)bytes_written >= iov->iov_len) {
+			bytes_written -= iov->iov_len;
+			iov++;
+			iovcnt--;
+		}
+
+		/*
+		 * Finally, we need to adjust the last iovec in case we have
+		 * performed a partial write.
+		 */
+		if (iovcnt && bytes_written) {
+			iov->iov_base = (char *) iov->iov_base + bytes_written;
+			iov->iov_len -= bytes_written;
+		}
+	}
+
+	return total_written;
+}
+
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
 {
 	char *p = buf;
diff --git a/wrapper.h b/wrapper.h
index 15ac3bab6e..27519b32d1 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -47,6 +47,15 @@ ssize_t read_in_full(int fd, void *buf, size_t count);
 ssize_t write_in_full(int fd, const void *buf, size_t count);
 ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
 
+/*
+ * Try to write all iovecs. Returns -1 in case an error occurred with a proper
+ * errno set, the number of bytes written otherwise.
+ *
+ * Note that the iovec will be modified as a result of this call to adjust for
+ * partial writes!
+ */
+ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt);
+
 static inline ssize_t write_str_in_full(int fd, const char *str)
 {
 	return write_in_full(fd, str, strlen(str));
diff --git a/write-or-die.c b/write-or-die.c
index 01a9a51fa2..5f522fb728 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -96,6 +96,14 @@ void write_or_die(int fd, const void *buf, size_t count)
 	}
 }
 
+void writev_or_die(int fd, struct iovec *iov, int iovlen)
+{
+	if (writev_in_full(fd, iov, iovlen) < 0) {
+		check_pipe(errno);
+		die_errno("writev error");
+	}
+}
+
 void fwrite_or_die(FILE *f, const void *buf, size_t count)
 {
 	if (fwrite(buf, 1, count, f) != count)
diff --git a/write-or-die.h b/write-or-die.h
index ff0408bd84..a045bdfaef 100644
--- a/write-or-die.h
+++ b/write-or-die.h
@@ -7,6 +7,7 @@ void fprintf_or_die(FILE *, const char *fmt, ...);
 void fwrite_or_die(FILE *f, const void *buf, size_t count);
 void fflush_or_die(FILE *f);
 void write_or_die(int fd, const void *buf, size_t count);
+void writev_or_die(int fd, struct iovec *iov, int iovlen);
 
 /*
  * These values are used to help identify parts of a repository to fsync.

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH 3/5] wrapper: properly handle MAX_IO_SIZE in writev(3p)

[Patrick Steinhardt]

Some systems like NonStop set a comparatively small `MAX_IO_SIZE`, which
limits the maximum number of bytes we're allowed to write in a single
call. We already handle this limit properly in `xwrite()`, but we have
recently introduced wrappers for writev(3p) where we don't. This will
cause the syscall to return EINVAL in case somebody passes an iovec
entry to writev(3p) that is larger than `MAX_IO_SIZE`.

Introduce a new function `xwritev()` that is similar to `xwrite()` in
that it handles such platform-specific nuances:

  - We only pass the leading iovec entries to writev(3p) that fit into
    `MAX_IO_SIZE`, pretending that the underlying syscall performed a
    short write. This mirrors how `xwrite()` chomps overly large
    requests before handing them to write(3p). As a consequence, callers
    will never see writev(3p)'s EINVAL error for requests whose summed
    length would overflow an ssize_t, but observe a short write instead.

  - If already the first iovec entry exceeds the limit we instead punt
    to `xwrite()`, which knows to handle this case for us.

  - We restart the underlying syscall on EINTR and EAGAIN, just like
    `xwrite()` does for write(3p).

Adapt `writev_in_full()` to use this new wrapper. With the retry logic
now living in `xwritev()`, the calling loop becomes the exact mirror
image of `write_in_full()`, which also retains the responsibility of
translating a zero-length write into ENOSPC.

Reported-by: Randall Becker <randall.becker@nexbridge.ca>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 wrapper.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
 wrapper.h |  1 +
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index be8fa575e6..561f9ee9c9 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -323,17 +323,54 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
 	return total;
 }
 
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)
+{
+	size_t allowed = MAX_IO_SIZE;
+	int i;
+
+	/*
+	 * Some platforms define a comparatively small `MAX_IO_SIZE` that
+	 * limits how many bytes can be written with a single call to
+	 * write(3p) or writev(3p); exceeding that limit causes the syscall to
+	 * fail with EINVAL. Just like xwrite() chomps overly large requests
+	 * for write(3p), pretend that the underlying writev(3p) performed a
+	 * short write by only passing along the leading iovec entries that
+	 * fit into that limit.
+	 */
+	for (i = 0; i < iovcnt; i++) {
+		if (iov[i].iov_len > allowed) {
+			/*
+			 * If the first buffer is larger than MAX_IO_SIZE,
+			 * let xwrite() deal with it.
+			 */
+			if (!i)
+				return xwrite(fd, iov->iov_base, iov->iov_len);
+			break;
+		}
+		allowed -= iov[i].iov_len;
+	}
+
+	while (1) {
+		ssize_t bytes_written = writev(fd, iov, i);
+		if (bytes_written < 0) {
+			if (errno == EINTR)
+				continue;
+			if (handle_nonblock(fd, POLLOUT, errno))
+				continue;
+		}
+
+		return bytes_written;
+	}
+}
+
 ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
 {
 	ssize_t total_written = 0;
 
 	while (iovcnt) {
-		ssize_t bytes_written = writev(fd, iov, iovcnt);
-		if (bytes_written < 0) {
-			if (errno == EINTR || errno == EAGAIN)
-				continue;
+		ssize_t bytes_written = xwritev(fd, iov, iovcnt);
+		if (bytes_written < 0)
 			return -1;
-		}
 		if (!bytes_written) {
 			errno = ENOSPC;
 			return -1;
diff --git a/wrapper.h b/wrapper.h
index 27519b32d1..a6287d7f4d 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_
 int xopen(const char *path, int flags, ...);
 ssize_t xread(int fd, void *buf, size_t len);
 ssize_t xwrite(int fd, const void *buf, size_t len);
+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);
 ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 int xdup(int fd);
 FILE *xfopen(const char *path, const char *mode);

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH 4/5] sideband: use writev(3p) to send pktlines

[Patrick Steinhardt]

Every pktline that we send out via `send_sideband()` currently requires
two syscalls: one to write the pktline's length, and one to send its
data. This typically isn't all that much of a problem, but under extreme
load the syscalls may cause contention in the kernel.

Refactor the code to instead use the newly introduced writev(3p) infra
so that we can send out the data with a single syscall. This reduces the
number of syscalls from around 133,000 calls to write(3p) to around
67,000 calls to writev(3p).

This change leads to a performance improvement for git-upload-pack(1),
but we have to cheat a bit to really make it measurable. Usually, the
time is strongly dominated by generating the packfile itself. But if we
precompute the pack and serve it via the pack-objects hook then we can
essentially eliminate that overhead. The following setup is executed in
the Git repository:

  $ cat >request <<-EOF
  0048want 5ce91c059e41090e7d2cffad39c04af8acf98dc1 side-band no-progress
  00000009done
  EOF
  $ echo 5ce91c059e41090e7d2cffad39c04af8acf98dc1 | git pack-objects --revs --stdout >pack
  $ cat >hook <<-EOF
  #!/bin/sh
  cat >/dev/null
  cat "$(pwd)"/pack
  EOF
  $ chmod u+x hook
  $ git -c uploadpack.packObjectsHook="$(pwd)"/hook upload-pack . <request

Benchmarking the last command leads to the following results:

  Benchmark 1: HEAD~
    Time (mean ± σ):     192.9 ms ±   0.6 ms    [User: 106.5 ms, System: 95.3 ms]
    Range (min … max):   191.7 ms … 194.1 ms    50 runs

  Benchmark 2: HEAD
    Time (mean ± σ):     141.1 ms ±   0.7 ms    [User: 63.2 ms, System: 86.6 ms]
    Range (min … max):   139.8 ms … 142.7 ms    50 runs

  Summary
    HEAD ran
      1.37 ± 0.01 times faster than HEAD~

This might not be impressive in absolute numbers when you also take into
account the time it takes to generate the packfile itself. But GitLab
(and supposedly other forges) have caching mechanisms in place that work
exactly like the above setup, where repeated incoming requests can be
served from the same cached packfile. And in those cases, the impact is
sizeable.

More importantly though, as hinted at above, GitLab has observed in the
past that with enough cache hits we eventually start to saturate a
semaphore in the Linux kernel itself in the pipe write path. This
bottleneck is being moved a bit by having to do less syscalls.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sideband.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sideband.c b/sideband.c
index 1523a53e1d..94e5b56172 100644
--- a/sideband.c
+++ b/sideband.c
@@ -441,6 +441,7 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
 	const char *p = data;
 
 	while (sz) {
+		struct iovec iov[2];
 		unsigned n;
 		char hdr[5];
 
@@ -450,12 +451,19 @@ void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_ma
 		if (0 <= band) {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 5);
 			hdr[4] = band;
-			write_or_die(fd, hdr, 5);
+			iov[0].iov_base = hdr;
+			iov[0].iov_len = 5;
 		} else {
 			xsnprintf(hdr, sizeof(hdr), "%04x", n + 4);
-			write_or_die(fd, hdr, 4);
+			iov[0].iov_base = hdr;
+			iov[0].iov_len = 4;
 		}
-		write_or_die(fd, p, n);
+
+		iov[1].iov_base = (void *) p;
+		iov[1].iov_len = n;
+
+		writev_or_die(fd, iov, ARRAY_SIZE(iov));
+
 		p += n;
 		sz -= n;
 	}

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH 5/5] fast-import: use writev(3p) to send cat-blob responses

[Patrick Steinhardt]

When answering a `cat-blob` command, `cat_blob()` issues three separate
calls to write(3p) on the cat-blob fd: one for the header line, one for
the full blob payload, and one for the trailing newline. Frontends like
git-filter-repo issue these commands in bulk, once per rewritten blob,
so the syscall overhead adds up.

Use `writev_in_full()` to send all three parts with a single syscall.

This can be benchmarked with the following setup:

    $ git cat-file --unordered --filter=object:type=blob
        --batch-check='cat-blob %(objectname)' --batch-all-objects >request
    $ git fast-import --cat-blob-fd=3 <request

Executing this with 100,000 objects in linux.git:

  Benchmark 1: HEAD~
    Time (mean ± σ):      1.320 s ±  0.003 s    [User: 1.154 s, System: 0.161 s]
    Range (min … max):    1.314 s …  1.324 s    10 runs

  Benchmark 2: HEAD
    Time (mean ± σ):      1.270 s ±  0.022 s    [User: 1.133 s, System: 0.132 s]
    Range (min … max):    1.209 s …  1.282 s    10 runs

  Summary
    HEAD ran
      1.04 ± 0.02 times faster than HEAD~

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fast-import.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index aa656c5195..48fda01c94 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3332,6 +3332,7 @@ static void cat_blob_write(const char *buf, unsigned long size)
 static void cat_blob(struct object_entry *oe, struct object_id *oid)
 {
 	struct strbuf line = STRBUF_INIT;
+	struct iovec iov[3];
 	unsigned long size;
 	enum object_type type = 0;
 	char *buf;
@@ -3365,10 +3366,21 @@ static void cat_blob(struct object_entry *oe, struct object_id *oid)
 	strbuf_reset(&line);
 	strbuf_addf(&line, "%s %s %"PRIuMAX"\n", oid_to_hex(oid),
 		    type_name(type), (uintmax_t)size);
-	cat_blob_write(line.buf, line.len);
+
+	/*
+	 * Write the header, the payload and the trailing newline with a
+	 * single writev(3p) call instead of three separate write(3p) calls.
+	 */
+	iov[0].iov_base = line.buf;
+	iov[0].iov_len = line.len;
+	iov[1].iov_base = buf;
+	iov[1].iov_len = size;
+	iov[2].iov_base = (void *) "\n";
+	iov[2].iov_len = 1;
+
+	if (writev_in_full(cat_blob_fd, iov, ARRAY_SIZE(iov)) < 0)
+		die_errno(_("write to frontend failed"));
 	strbuf_release(&line);
-	cat_blob_write(buf, size);
-	cat_blob_write("\n", 1);
 	if (oe && oe->pack_id == pack_id) {
 		last_blob.offset = oe->idx.offset;
 		strbuf_attach(&last_blob.data, buf, size, size + 1);

-- 
2.55.0.313.g8d093f411d.dirty

Re: [PATCH 1/5] compat/posix: introduce writev(3p) wrapper

[Simon Richter]

Hi,

> +		if (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||
> +		    iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {

That feels like it could overflow.

    Simon

Re: [PATCH 0/5] Reintroduce writev(3p)

[Johannes Sixt]

Am 16.07.26 um 09:52 schrieb Patrick Steinhardt:
> this patch series reintroduces the writev(3p) wrapper. This wrapper was
> originally introduced as part of Git 2.54 [1], but was ejected due to
> issues on NonStop [2].

Please don't call the function "writev" so that nobody associates it
with the guarantees that only POSIX provides, but none of the
emulations. Call it "write_gather", for example.

Also, clearly document that its only purpose is to reduce sequences of
write() calls to a single function call, but that the additional writev
guarantees are not needed.

A range-diff to the earlier round would have been very helpful.

-- Hannes

Re: [PATCH 1/5] compat/posix: introduce writev(3p) wrapper

[Junio C Hamano]

Simon Richter <Simon.Richter@hogyros.de> writes:

> Hi,
>
>> +		if (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||
>> +		    iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {
>
> That feels like it could overflow.

Isn't it checking if it would overflow (and dying if so)?

Ah, wait.  The addition "(iov[i].iov_len + sum)" can indeed wrap
around, and comparing it with the maximum value of ssize_t wouldn't
catch that.  Is that what you mean?

Would something like this:

    if (maximum_signed_value_of_type(ssize_t) < iov[i].iov_len ||
	iov[i].iov_len + sum < iov[i].iov_len ||
	maximum_signed_value_of_type(ssize_t) < iov[i].iov_len + sum)

work better to catch the three cases independently?

 (1) The value is already too large on its own.
 (2) Adding them together would cause an unsigned wrap-around.
 (3) The sum does not wrap around, but it exceeds the maximum
     representable value of ssize_t anyway.

Re: [PATCH 1/5] compat/posix: introduce writev(3p) wrapper

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Simon Richter <Simon.Richter@hogyros.de> writes:
>
>> Hi,
>>
>>> +		if (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||
>>> +		    iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {
>>
>> That feels like it could overflow.
>
> Isn't it checking if it would overflow (and dying if so)?
>
> Ah, wait.  The addition "(iov[i].iov_len + sum)" can indeed wrap
> around, and comparing it with the maximum value of ssize_t wouldn't
> catch that.  Is that what you mean?
>
> Would something like this:
>
>     if (maximum_signed_value_of_type(ssize_t) < iov[i].iov_len ||
> 	iov[i].iov_len + sum < iov[i].iov_len ||
> 	maximum_signed_value_of_type(ssize_t) < iov[i].iov_len + sum)
>
> work better to catch the three cases independently?
>
>  (1) The value is already too large on its own.
>  (2) Adding them together would cause an unsigned wrap-around.
>  (3) The sum does not wrap around, but it exceeds the maximum
>      representable value of ssize_t anyway.

Actually, looking at it again, I think the original code is safe
after all, because:

 * "sum", even though it is a size_t, is checked inside the loop to
   ensure it stays below the maximum value of ssize_t each time it
   gets a new value.
 * iov[i].iov_len is checked to ensure it does not exceed the
   maximum value of ssize_t by the first part of the condition.

If both values are less than or equal to the maximum value of
ssize_t, their sum is at most twice that limit.  For an N-bit
size_t, this sum is at most (2^N - 2), which can be computed safely
without any unsigned wrap-around.

So...?