Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper

[Jeff King <peff@peff.net>]

On Wed, Mar 04, 2026 at 02:01:18PM -0800, Junio C Hamano wrote:

> Because we do not check the accumulation of bytes_written in the two
> accumulator variables inside the inner loop, it is very possible for
> total_written to wraparound size_t and end up below the largest
> value possible to be stored in ssize_t type.

Coverity complained about it, too.

> IOW, the cast_size_t_to_ssize_t() introduced in the previous step is
> pointless, isn't it?
> 
> According to [*1*], the real
> 
>     ssize_t writev(int fd, const struct iovec *iov, int iovcnt)
> 
> is supposed to report error with errno set to EINVAL when the sum of
> iov_len member of the iov[] array elements exceed half of the
> maximum size_t.
> 
>        EINVAL The sum of the iov_len values overflows an ssize_t value.
> 
> So instead of dying with cast_size_t_to_ssize_t(), we probably would
> want the check done in a more stupid and straight-forward way?
> Adding up iov[i].iov_len while the addition would not wraparound in
> each and every step, and return error with EINVAL before attempting
> even a single call to xwrite(), and then have the above double loop
> that does not care about integer wraparound at all, and return
> total_written with simple cast to (ssize_t)?

I like that writev() can work as a drop-in replacement for write() at
the lowest level. But given that our main use is likely to be pkt-lines,
I do kind of wonder if we should just try to be more clever in forming
our buffers. That makes all of the portability and compat questions go
away (and gives the benefit to platforms that don't even have writev).

E.g., the somewhat ugly patch below is enough to do single write()s for
the pack data in upload-pack. I think if the pkt-writing API were less
ad-hoc (and had an actual buffer in "struct packet_writer"), this could
be made a bit more universal.

diff --git a/upload-pack.c b/upload-pack.c
index 1b1c81ea63..d01b547b10 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -179,11 +179,32 @@ static void reset_timeout(unsigned int timeout)
 	alarm(timeout);
 }
 
-static void send_client_data(int fd, const char *data, ssize_t sz,
-			     int use_sideband)
+#define send_client_data(fd, data, sz, use_sideband) \
+	send_client_data_1((fd), (data), (sz), (use_sideband), NULL)
+static void send_client_data_1(int fd, const char *data, ssize_t sz,
+			       int use_sideband, char *hdr_buf)
 {
 	if (use_sideband) {
-		send_sideband(1, fd, data, sz, use_sideband);
+		/*
+		 * If we don't have a contiguous header buf, or if the data is
+		 * too big for a single packet (which I don't think happens
+		 * with any of our current callers), bail to the old-style
+		 * send_sideband.
+		 */
+		if (!hdr_buf || sz > LARGE_PACKET_DATA_MAX - 1) {
+			send_sideband(1, fd, data, sz, use_sideband);
+			return;
+		}
+
+		if (hdr_buf + 5 != data)
+			BUG("hdr_buf should be contiguous with pkt data");
+
+		if (use_sideband < 0)
+			BUG("negative sideband!?");
+
+		set_packet_header(hdr_buf, sz + 5);
+		hdr_buf[4] = 1; /* sideband 1 */
+		write_or_die(fd, hdr_buf, sz + 5);
 		return;
 	}
 	if (fd == 3)
@@ -211,7 +232,14 @@ struct output_state {
 	 * sideband-64k the band designator takes up 1 byte of space. Because
 	 * relay_pack_data keeps the last byte to itself, we make the buffer 1
 	 * byte bigger than the intended maximum write size.
+	 *
+	 * However, we also reserve 5 bytes before the buffer so we can format
+	 * a packet header and send it with a single write(). We may be pushing
+	 * our luck on assuming there will be no padding here (though we do
+	 * check it with a BUG(), but there are other ways to write it
+	 * (e.g., a single large buffer with "buffer" as a pointer into it).
 	 */
+	char hdr[5];
 	char buffer[(LARGE_PACKET_DATA_MAX - 1) + 1];
 	int used;
 	unsigned packfile_uris_started : 1;
@@ -284,7 +312,7 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,
 		return readsz;
 
 	if (os->used > 1) {
-		send_client_data(1, os->buffer, os->used - 1, use_sideband);
+		send_client_data_1(1, os->buffer, os->used - 1, use_sideband, os->hdr);
 		os->buffer[0] = os->buffer[os->used - 1];
 		os->used = 1;
 	} else {

-Peff