From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 14 Oct 2026 11:37:59 GMT
Subject: Re: [PATCH 3/5] wrapper: reject insanely large byte counts when reading/writing
Message-ID: <96c1d112-1b05-4663-90c6-33374dccd764@gmail.com>
In-Reply-To: <b1ed701935027a83574fae753e5cc11d23f0ea4b.1791958171.git.gitgitgadget@gmail.com>

Hi Johannes

On 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Both the `read_in_full()` as well as the `write_in_full()` function
> accumulate the bytes transferred count in a variable of type `ssize_t`,
> but accept a `size_t` count parameter. When count exceeds `SSIZE_MAX`,
> that total count overflows after processing `SSIZE_MAX` bytes, producing
> a negative return value that callers interpret as an error (or worse, a
> small positive value after further wrapping).
> > In practice, git never passes such large counts because file sizes and
> buffer allocations are bounded well below SSIZE_MAX on all target
> platforms. Are we sure about that? In the past we've had bug fixes such as c14e5a1a50 (transport-helper: use xread instead of read, 2019-01-03)
on platforms where ssize_t is narrower that size_t (POSIX does not require them to be the same width). I've cc'd Randall to see if ssize_t is still narrower than size_t on the NonStop platforms git supports.

Thanks

Phillip

> The POSIX specification for `read()` and `write()` also
> limits single operations to `SSIZE_MAX` bytes, and git's
> `xread()`/`xwrite()` wrappers further clamp to `MAX_IO_SIZE`. But the
> accumulation in the `*_in_full()` wrappers means that even with per-call
> clamping, total can exceed `SSIZE_MAX` when count does.
> > Reject a total count that exceeds the range of `ssize_t` upfront with
> `EINVAL` rather than silently producing wrong return values, matching
> the POSIX convention for invalid arguments.
> > Pointed out by Coverity.
> > Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   git-compat-util.h |  4 ++++
>   wrapper.c         | 10 ++++++++++
>   2 files changed, 14 insertions(+)
> > diff --git a/git-compat-util.h b/git-compat-util.h
> index a0f901ce79..afce8b85b8 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -740,6 +740,10 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)
>    */
>   #define DEFAULT_IO_BUFFER_SIZE (128 * 1024)
>   > +#ifndef SSIZE_MAX
> +# define SSIZE_MAX ((ssize_t)(((size_t)-1) >> 1))
> +#endif
> +
>   #ifdef HAVE_ALLOCA_H
>   # include <alloca.h>
>   # define xalloca(size)      (alloca(size))
> diff --git a/wrapper.c b/wrapper.c
> index 05a9cd369c..ea9c14f834 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -288,6 +288,11 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
>   	char *p = buf;
>   	ssize_t total = 0;
>   > +	if (count > SSIZE_MAX) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
>   	while (count > 0) {
>   		ssize_t loaded = xread(fd, p, count);
>   		if (loaded < 0)
> @@ -307,6 +312,11 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
>   	const char *p = buf;
>   	ssize_t total = 0;
>   > +	if (count > SSIZE_MAX) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
>   	while (count > 0) {
>   		ssize_t written = xwrite(fd, p, count);
>   		if (written < 0)