Address overflows pointed out by Coverity

This patch series is part of my effort to bring Git for Windows' Coverity reports down to zero; Previous patch series have been merged in 347fe8a91b5 (Merge branch 'js/coverity-fixes', 2026-07-16), 0b75ef5168c (Merge branch 'js/coverity-fixes-null-safety', 2026-07-19), fcc8c931b53 (Merge branch 'js/coverity-unchecked-returns-fix', 2026-08-24) and 8bc9c7fcfd8 (Merge branch 'js/coverity-fixes', 2026-10-03).

Johannes Schindelin (5):
archive-tar: guard get_path_prefix against zero-length input
http: use size_t for byte position in fwrite_sha1_file
wrapper: reject insanely large byte counts when reading/writing
git-compat-util: add signed_mult_overflows and mult_overflows
macros
date: avoid overflow in approxidate relative-time multiplication

 archive-tar.c     |  2 ++
 date.c            | 26 +++++++++++++++++++++--
 git-compat-util.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 http.c            |  2 +-
 wrapper.c         | 10 +++++++++
 5 files changed, 91 insertions(+), 3 deletions(-)

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2253%2Fdscho%2Fcoverity-fixes-overflow-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2253/dscho/coverity-fixes-overflow-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2253

gitgitgadget

From: Johannes Schindelin <[email protected]>

The get_path_prefix() function uses a do ... while() loop that unconditionally decrements the size_t index variable before checking the loop condition. When that variable is 0 on entry, the decrement wraps to SIZE_MAX, and path[SIZE_MAX] is an out-of-bounds read.

The current caller (write_tar_entry()) only invokes this function when pathlen > sizeof(header.name) i.e. when pathlen is at least 100. Therefore, the bug is unreachable in practice.

However, the function's interface accepts arbitrary pathlen and maxlen values, and a future caller or a maxlen of 0 could trigger the wrap, so let's add a check for that.

Pointed out by Coverity.

---
 archive-tar.c | 2 ++
 1 file changed, 2 insertions(+)

index a1c66024d4..2da6e55484 100644
@@ -210,6 +210,8 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
 		i--;
 	if (i > maxlen)
 		i = maxlen;
+	if (!i)
+		return 0;
 	do {
 		i--;
 	} while (i > 0 && path[i] != '/');

gitgitgadget

From: Johannes Schindelin <[email protected]>

The fwrite_sha1_file() function tracks the number of bytes written via an int variable, but the total size to write is of type size_t. When the HTTP response body exceeds INT_MAX (~2GB), this causes an overflow and a heap under-read, then writes that data to the local object file.

The hash check afterward would reject the corrupt object (so no bad data enters the object store), but the under-read itself could leak adjacent heap contents to the temporary file. In practice this is unlikely because curl delivers data in small chunks (typically 16KB per callback invocation), so in practice, the total size is far below INT_MAX in a single call. However, the function interface accepts arbitrary sizes, and the accumulation across the loop means that the int variable grows across calls within a single invocation, not across callbacks.

Change the type from int to size_t to match the type of the total size. This is a hardening fix: the attack surface (a malicious HTTP server triggering a >2GB single-callback delivery) is theoretical under normal curl configurations, but the type mismatch is a genuine defect.

Pointed out by Coverity.

---
 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4af3c29c76..809471f436 100644
@@ -2848,7 +2848,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
 {
 	unsigned char expn[4096];
 	size_t size = eltsize * nmemb;
-	int posn = 0;
+	size_t posn = 0;
 	struct http_object_request *freq = data;
 	struct active_request_slot *slot = freq->slot;
 

gitgitgadget

I agree that changing the type of posn from int to size_t is the right move.

But I wondered what happens when (size - posn) is larger than half the range of the ssize_t type early in the loop. After all, the proposed commit log message worries about the case where HTTP response body exceeds 2GB.

Shouldn't the loop clamp the bytecount passed to xwrite() below whatever would fit in ssize_t when successful? It may end up making two calls of xwrite, writing 1.5GB each, to write out 3GB payload, but that would be better than attempting to write 3GB, succeed, and be fooled by ssize_t that is narrower by size_t by at least one bit that we got a negative return from xwrite() and return an error from here, when in reality we have written them all?

    ... goes and looks ... 

Ah, nevermind. The big comment in git-compat-util.h before MAX_IO_SIZE clarifies all of the above. In short, xwrite() takes care of clamping, so its callers including this code do not have to worry about it.

Hi,

It is a position in a file that is being read as binary data[1], so off_t would be the best choice I think.

That is still signed, but it can't overflow, because opening large files requires O_LARGEFILE, which is available only if large file support is enabled, which conveniently makes sure that off_t is 64 bit.

Simon

[1] to make a distinction from the use case for fpos_t

Hmm, interesting. The function writes out eltsize * nmemb (both are of type size_t) bytes, so the total could theoretically exceed the range that fits size_t.

But the thing is that posn is used this way

		ssize_t retval = xwrite(freq->localfile,
					(char *) ptr + posn, size - posn);

to offset into a contiguous in-core region that starts at ptr. I am not sure if (ptr + posn) is still valid for posn that is too large to fit in size_t, though.

To put it differently, we are writing out what is in the contiguous in-core region and posn runs up to the allocation size of that region, which should be measured in size_t, no?

Interesting.

The function writes out eltsize * nmemb bytes (both of type size_t), which means the product could theoretically exceed the range of size_t. However, posn is used in xwrite() to offset into a contiguous in-core buffer starting at ptr:

        ssize_t retval = xwrite(freq->localfile,
                                (char *) ptr + posn,
                                size - posn);

Because we are indexing into an allocated in-core region, any valid offset must fit within size_t. If posn were to exceed the bounds of size_t, the pointer arithmetic ptr + posn would already be invalid. As posn only runs up to the allocation size of the region, it indeed should be measured in size_t, no?

From: Johannes Schindelin <[email protected]>

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. 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.

---
 git-compat-util.h |  4 ++++
 wrapper.c         | 10 ++++++++++
 2 files changed, 14 insertions(+)

index a0f901ce79..afce8b85b8 100644
@@ -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))

index 05a9cd369c..ea9c14f834 100644
@@ -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)

gitgitgadget

Hi Johannes

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

From: Johannes Schindelin <[email protected]>

unsigned_mult_overflows() does not cover signed arithmetic, but types such as time_t can be signed or unsigned. Add signed and type-generic guards so callers can check multiplication overflow in either case.

Overflow depends on the product type selected by C's usual arithmetic conversions. Products equal to the signed minimum are representable and must not be rejected.

---
 git-compat-util.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

index afce8b85b8..077fa94873 100644
@@ -85,9 +85,27 @@ struct strbuf;
 
 #define bitsizeof(x)  (CHAR_BIT * sizeof(x))
 
+#if GIT_GNUC_PREREQ(3, 1)
+#define is_unsigned_type(a) ((__typeof__(a))-1 >= 0)
+#elif __STDC_VERSION__ >= 201112L
+#define is_unsigned_type(a) _Generic((a), \
+	unsigned char: 1, \
+	unsigned short: 1, \
+	unsigned int: 1, \
+	unsigned long: 1, \
+	unsigned long long: 1, \
+	_Bool: 1, \
+	char: (CHAR_MIN == 0), \
+	default: 0)
+#endif
+
 #define maximum_signed_value_of_type(a) \
     (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))
 
+/* Account for the extra negative value on two's-complement targets. */
+#define minimum_signed_value_of_type(a) \
+    (-maximum_signed_value_of_type(a) - (INTMAX_MIN < -INTMAX_MAX))
+
 #define maximum_unsigned_value_of_type(a) \
     (UINTMAX_MAX >> (bitsizeof(uintmax_t) - bitsizeof(a)))
 

@@ -111,6 +129,38 @@ struct strbuf;
 #define unsigned_mult_overflows(a, b) \
     ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))
 
+/*
+ * Returns true if the multiplication of "a" and "b" will
+ * overflow or underflow a signed type. The types of "a" and "b"
+ * must match and must be signed. Note that this macro evaluates
+ * both "a" and "b" twice!
+ */
+#define signed_mult_overflows(a, b) \
+    (((a) > 0 && (b) > 0 && (a) > maximum_signed_value_of_type(a) / (b)) || \
+     ((a) < 0 && (b) < 0 && (a) < maximum_signed_value_of_type(a) / (b)) || \
+     ((a) > 0 && (b) < 0 && (b) < minimum_signed_value_of_type(a) / (a)) || \
+     ((a) < 0 && (b) > 0 && (a) < minimum_signed_value_of_type(b) / (b)))
+
+/*
+ * Returns true if the product of "a" and "b", after the usual arithmetic
+ * conversions, cannot be represented in the multiplication expression's
+ * type. The operands must be integers and may be evaluated more than once.
+ */
+#if GIT_GNUC_PREREQ(3, 1)
+#define mult_overflows(a, b) \
+    (is_unsigned_type((a) * (b)) \
+     ? unsigned_mult_overflows((__typeof__((a) * (b)))(a), \
+			       (__typeof__((a) * (b)))(b)) \
+     : signed_mult_overflows((__typeof__((a) * (b)))(a), \
+			     (__typeof__((a) * (b)))(b)))
+#else
+/*
+ * This implementation needs GNU `typeof` to convert both operands to the
+ * multiplication's type. Skip the check without that extension.
+ */
+#define mult_overflows(a, b) 0
+#endif
+
 /*
  * Returns true if the left shift of "a" by "shift" bits will
  * overflow. The type of "a" must be unsigned.

gitgitgadget

Hi Johannes

It would have been nice to mention that the type-generic macro is gcc only so we cannot rely on this for safety checks.

At least twice I think

As the macro relies on both operands being the same width, can we use BUILD_ASSERT_OR_ZERO() to ensure sizeof(a) == sizeof(b) here?

From: Johannes Schindelin <[email protected]>

The approxidate_alpha() function multiplies a time unit length (e.g., 604800 for weeks) by a user-supplied number to compute a time offset in seconds. Both operands are int, and the product is passed to update_tm() as time_t. For inputs like "99999 weeks ago", the int multiplication overflows (604800 * 99999 is roughly 60 billion, far beyond INT_MAX), producing a wrapped value that results in a wrong date.

Use the checked time_t multiplication so oversized products do not overflow or wrap around.

On platforms where time_t is unsigned (and where the pre-processor allows us to determine this fact), reject counts that are too large and report a parse error. Narrowing them first could otherwise produce negative or truncated values that become bogus offsets. Leave signed time_t count parsing unchanged.

Pointed out by Coverity.

---
 date.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

index 014065b419..0d65804d2f 100644
@@ -1298,7 +1298,18 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 	while (tl->type) {
 		size_t len = strlen(tl->type);
 		if (match_string(date, tl->type) >= len-1) {
-			update_tm(tm, now, tl->length * *num);
+			time_t length = tl->length, num_t = *num;
+			time_t seconds;
+#ifndef is_unsigned_type
+			seconds = length * num_t;
+#else
+			time_t max_val = is_unsigned_type(length)
+				? maximum_unsigned_value_of_type(length)
+				: maximum_signed_value_of_type(length);
+			seconds = mult_overflows(length, num_t)
+				? max_val : length * num_t;
+#endif
+			update_tm(tm, now, seconds);
 			*num = 0;
 			*touched = 1;
 			return end;

@@ -1368,8 +1379,15 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,
 	}
 
 	/* Accept zero-padding only for small numbers ("Dec 02", never "Dec 0002") */
-	if (date[0] != '0' || end - date <= 2)
+	if (date[0] != '0' || end - date <= 2) {
+#ifdef is_unsigned_type
+		if (is_unsigned_type((time_t)0) &&
+		    (number > INT_MAX ||
+		     number > maximum_unsigned_value_of_type((time_t)0)))
+			return NULL;
+#endif
 		*num = number;
+	}
 	return end;
 }
 

@@ -1398,6 +1416,10 @@ static timestamp_t approxidate_str(const char *date,
 		if (isdigit(c)) {
 			pending_number(&tm, &number);
 			date = approxidate_digit(date-1, &tm, &number, time_sec);
+			if (!date) {
+				*error_ret = 1;
+				return 0;
+			}
 			touched = 1;
 			continue;
 		}

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Does this mean that "git log -p" in a project that were made on reasonably sized platforms and then cloned to my box may cause the history traversal to stop with an error when a commit has author or committer timestamp that would not fit the time_t on my box?

It would have been horrible regression than showing the entire history with patches with bogus timestamp if it were the case.

But I do not think that is what happens. approxidate_careful() is only used to parse input by local user (e.g., "git commit --date="...", "git log @{...}"), and the changes in this patch would not affect behaviours of other uses of timestamps, so we are safe.

Hi Johannes

Signed integer overflow is undefined so we don't necessarily get a wrapped value - the whole program is invalid.

It looks like "length" is always positive, can the user supplied number ever be negative?

This is only true when compiling with gcc or clang though because mult_overflows() always returns 0 on other compilers, so we're still exposed to undefined behavior when compiling with a different compiler on platforms where time_t is signed. I wonder if it would be better to perform the multiplication on a known type and then check that the result can be losslessly converted to time_t

	intmax_t length tl->length, num_t = *num, result;

	if (signed_mult_overflows(length, num_t))
		// error
	result = length * num_t;
	if ((intmax_t)((time_t)result) != result ||
	    (result < 0 && (time_t)result > 0))
		// error

that would avoid overflow on all platforms, regardless of the compiler and regardless of the type of time_t. Having mult_overflows() only implemented for gcc seems like a footgun waiting to happen when we're typically checking for overflow to avoid some form of bad behavior.

I'm a bit confused what this is trying to do - why can't we check this when time_t is signed?

	if ((timestamp_t)((int)number) != number)
		return NULL

	return *val = value;

Ensures "value" fits in "val" without truncation, we could also add a check that value is positive when time_t is unsigned if we wanted.

Thanks

Phillip

Nice! Can Coverty run as part of GitHub CI? If so, once we have it down to zero, we can enable it to stop issues from creeping back in over time.

Harald

"Johannes Schindelin via GitGitGadget" <[email protected]>

It has been quite a while since we saw any activity on this thread.

The review discussion on patch [2/5] seems settled and I do not think the step needs any more work, but the review comments on patches [3/5], [4/5], and [5/5] are still awaiting a response from the author. The topic has been in the "Waiting for response" state since around Oct 17 (more than 20 days), and without any further activity, I will let it expire from my tree before long.

Thanks.