Repository navigation
Conversation
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. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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. Assisted-by: Claude Opus 4.6 Signed-off-by: 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. 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>
`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. Assisted-by: Claude Opus 4.6, GPT-6 and GPT-6.1 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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. Assisted-by: Claude Opus 4.6 and GPT-6.1 Sol Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
coverity-fixes-overflow
branch
from
October 13, 2026 12:36
8254f09 to
a4e602b
Compare
Member
Author
|
/submit |
|
Submitted as pull.2253.git.1791958171.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -740,6 +740,10 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b) | |||
| */ | |||
There was a problem hiding this comment.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
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)|
User |
|
This patch series was integrated into seen via git@bcbbca3. |
| @@ -1298,7 +1298,18 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm | |||
| while (tl->type) { | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> 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.
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.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> date.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/date.c b/date.c
> index 014065b419..0d65804d2f 100644
> --- a/date.c
> +++ b/date.c
> @@ -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;
> }| @@ -85,9 +85,27 @@ struct strbuf; | |||
|
|
|||
There was a problem hiding this comment.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
Hi Johannes
On 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > `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.
It would have been nice to mention that the type-generic macro is gcc only so we cannot rely on this for safety checks.
> #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))
Do we support any platforms that do not use two's complement arithmetic? C23 makes it mandatory so presumably there are not may contemporary platforms out there that do not use it.
> +/*
> + * 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!
At least twice I think
> + */
> +#define signed_mult_overflows(a, b) \
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?
> + (((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)))
Thanks
Phillip
> +
> +/*
> + * 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.| @@ -1298,7 +1298,18 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm | |||
| while (tl->type) { | |||
There was a problem hiding this comment.
Phillip Wood wrote on the Git mailing list (how to reply to this email):
Hi Johannes
On 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 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.
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?
> Use the checked `time_t` multiplication so oversized products do not
> overflow or wrap around.
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.
> 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.
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
> Pointed out by Coverity.
> > Assisted-by: Claude Opus 4.6 and GPT-6.1 Sol
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> date.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
> > diff --git a/date.c b/date.c
> index 014065b419..0d65804d2f 100644
> --- a/date.c
> +++ b/date.c
> @@ -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;
> }|
This branch is now known as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 347fe8a (Merge branch 'js/coverity-fixes', 2026-07-16), 0b75ef5 (Merge branch 'js/coverity-fixes-null-safety', 2026-07-19), fcc8c93 (Merge branch 'js/coverity-unchecked-returns-fix', 2026-08-24) and 8bc9c7f (Merge branch 'js/coverity-fixes', 2026-10-03).
cc: Phillip Wood phillip.wood123@gmail.com