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

On Thu, Jul 16, 2026 at 01:44:18PM -0700, Junio C Hamano wrote:
> Junio C Hamano <[email protected]> writes:
> 
> > Simon Richter <[email protected]> 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...?

Yeah, I think your analysis is correct. It's quite subtle though, so
maybe we should make this a bit more explicit? Something like the
following patch for example:

diff --git a/compat/writev.c b/compat/writev.c
index ab2e223634..960673861d 100644
--- a/compat/writev.c
+++ b/compat/writev.c
@@ -12,6 +12,7 @@ ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
 	 */
 	for (int i = 0; i < iovcnt; i++) {
 		if (iov[i].iov_len > maximum_signed_value_of_type(ssize_t) ||
+		    unsigned_add_overflows(iov[i].iov_len, sum) ||
 		    iov[i].iov_len + sum > maximum_signed_value_of_type(ssize_t)) {
 			errno = EINVAL;
 			return -1;

I doubt the performance overhead of this additional check is really
going to matter :)

Patrick