Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 09, 2026 at 09:42:42AM -0700, Junio C Hamano wrote:
>
>> > +	for (i = 0, total_length = 0; i < iovcnt; i++) {
>> > +		if (unsigned_add_overflows(total_length, iov[i].iov_len))
>> > +			break;
>> 
>> We add .iov_len up in this first loop, because we do not want to
>> bust writev(3p)'s limit.
>> 
>> 	EINVAL The sum of the iov_len values in the iov array would
>>               overflow an ssize_t.
>> 
>> cf. https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html
>> 
>> As the width of ssize_t in bits can be a lot smaller than size_t,
>> the above "unsigned_add_overflows() triggers way too late for the
>> check to matter, no?
>
> I think it is correct as-is.
>
> The real check against ssize_t is later, when we compare total_length to
> MAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure
> we do not overflow size_t when counting up the total (and if we do, we
> _know_ we are going to overflow ssize_t, which must be smaller).

But then what happens after it breaks out of the loop?  We cannot be
at i==0, so let's say we have a reasonably small iov[0] and iov[1]
that is so large and makes size_t wraparound.  We break out here,
and then send the iov[0] with writev().  But have we checked if
iov[0] is under MAX_IO_SIZE in that case before calling writev()?

> I think this can be made more clear by counting down allowable bytes
> instead of up. I'll show an example in a second.
>
>> > +	}
>> > +
>> > +	if (i < iovcnt) {
>> > +		/*
>> > +		 * The first entry exceeds MAX_IO_SIZE, so we pass it to
>> > +		 * xwrite, which knows to handle this case.
>> > +		 */
>> > +		if (!i)
>> > +			return xwrite(fd, iov->iov_base, iov->iov_len);
>> 
>> Ben has a similar comment, but it would be easier to see the
>> correspondence if you rephrase the comment perhaps like
>> [...]
>
> Me three. I think this can be made more clear if we bail to xwrite()
> immediately in the loop. So together with the count-down, something
> like:
>
>    ssize_t allowed = MAX_IO_SIZE;
>    int i;
>
>    for (i = 0; i < iovcnt; i++) {
> 	if (iov[i].iov_len > allowed) {
> 		if (!i)
> 			return xwrite(fd, iov->iov_base, iov_len);
> 		break;
> 	}
> 	allowed -= iov[i].iov_len;
>   }
>   return writev(fd, iov, i);

I agree that this is much simpler to follow.