{"thread":{"id":"66527","subject":"[PATCH 0/5] Address overflows pointed out by Coverity","startedAt":"2026-10-14T06:09:26Z","lastAt":"2026-10-17T04:11:52Z","messageCount":16,"participants":["Johannes Schindelin via GitGitGadget","Phillip Wood","Junio C Hamano","Patrick Steinhardt","Harald Nordgren","Simon Richter"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"554944","messageId":"pull.2253.git.1791958171.gitgitgadget@gmail.com","threadId":"66527","inReplyTo":null,"subject":"[PATCH 0/5] Address overflows pointed out by Coverity","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-14T06:09:26Z","receivedAt":"2026-10-14T06:09:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series is part of my effort to bring Git for Windows' Coverity\nreports down to zero; Previous patch series have been merged in 347fe8a91b5\n(Merge branch 'js/coverity-fixes', 2026-07-16), 0b75ef5168c (Merge branch\n'js/coverity-fixes-null-safety', 2026-07-19), fcc8c931b53 (Merge branch\n'js/coverity-unchecked-returns-fix', 2026-08-24) and 8bc9c7fcfd8 (Merge\nbranch 'js/coverity-fixes', 2026-10-03).\n\nJohannes Schindelin (5):\n archive-tar: guard get_path_prefix against zero-length input\n http: use `size_t` for byte position in fwrite_sha1_file\n wrapper: reject insanely large byte counts when reading/writing\n git-compat-util: add `signed_mult_overflows` and `mult_overflows`\n macros\n date: avoid overflow in approxidate relative-time multiplication\n\n archive-tar.c | 2 ++\n date.c | 26 +++++++++++++++++++++--\n git-compat-util.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++\n http.c | 2 +-\n wrapper.c | 10 +++++++++\n 5 files changed, 91 insertions(+), 3 deletions(-)\n\n\nbase-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2253%2Fdscho%2Fcoverity-fixes-overflow-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2253/dscho/coverity-fixes-overflow-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2253\n-- \ngitgitgadget\n\n"},{"id":"554945","messageId":"dcd319476add64e892e517faa71966f09010029a.1791958171.git.gitgitgadget@gmail.com","threadId":"66527","inReplyTo":"pull.2253.git.1791958171.gitgitgadget@gmail.com","subject":"[PATCH 1/5] archive-tar: guard get_path_prefix against zero-length input","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-14T06:09:27Z","receivedAt":"2026-10-14T06:09:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `get_path_prefix()` function uses a `do ... while()` loop that\nunconditionally decrements the `size_t` index variable before checking\nthe loop condition. When that variable is 0 on entry, the decrement\nwraps to `SIZE_MAX`, and `path[SIZE_MAX]` is an out-of-bounds read.\n\nThe current caller (`write_tar_entry()`) only invokes this function when\n`pathlen > sizeof(header.name)` i.e. when `pathlen` is at least 100.\nTherefore, the bug is unreachable in practice.\n\nHowever, the function's interface accepts arbitrary pathlen and maxlen\nvalues, and a future caller or a maxlen of 0 could trigger the wrap, so\nlet's add a check for that.\n\nPointed out by Coverity.\n\nAssisted-by: Claude Opus 4.6\nSigned-off-by: Johannes Schindelin \n---\n archive-tar.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex a1c66024d4..2da6e55484 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -210,6 +210,8 @@ static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)\n \t\ti--;\n \tif (i > maxlen)\n \t\ti = maxlen;\n+\tif (!i)\n+\t\treturn 0;\n \tdo {\n \t\ti--;\n \t} while (i > 0 && path[i] != '/');\n-- \ngitgitgadget\n\n\n"},{"id":"554946","messageId":"70a909de18e8cd8d282439fb51a9399baa8c7be8.1791958171.git.gitgitgadget@gmail.com","threadId":"66527","inReplyTo":"pull.2253.git.1791958171.gitgitgadget@gmail.com","subject":"[PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-14T06:09:28Z","receivedAt":"2026-10-14T06:09:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `fwrite_sha1_file()` function tracks the number of bytes written via\nan `int` variable, but the total size to write is of type `size_t`. When\nthe HTTP response body exceeds INT_MAX (~2GB), this causes an overflow\nand a heap under-read, then writes that data to the local object file.\n\nThe hash check afterward would reject the corrupt object (so no bad data\nenters the object store), but the under-read itself could leak adjacent\nheap contents to the temporary file. In practice this is unlikely\nbecause curl delivers data in small chunks (typically 16KB per callback\ninvocation), so in practice, the total size is far below INT_MAX in a\nsingle call. However, the function interface accepts arbitrary sizes,\nand the accumulation across the loop means that the `int` variable grows\nacross calls within a single invocation, not across callbacks.\n\nChange the type from `int` to `size_t` to match the type of the total\nsize. This is a hardening fix: the attack surface (a malicious HTTP\nserver triggering a >2GB single-callback delivery) is theoretical under\nnormal curl configurations, but the type mismatch is a genuine defect.\n\nPointed out by Coverity.\n\nAssisted-by: Claude Opus 4.6\nSigned-off-by: Johannes Schindelin \n---\n http.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 4af3c29c76..809471f436 100644\n--- a/http.c\n+++ b/http.c\n@@ -2848,7 +2848,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n {\n \tunsigned char expn[4096];\n \tsize_t size = eltsize * nmemb;\n-\tint posn = 0;\n+\tsize_t posn = 0;\n \tstruct http_object_request *freq = data;\n \tstruct active_request_slot *slot = freq->slot;\n \n-- \ngitgitgadget\n\n\n"},{"id":"554947","messageId":"b1ed701935027a83574fae753e5cc11d23f0ea4b.1791958171.git.gitgitgadget@gmail.com","threadId":"66527","inReplyTo":"pull.2253.git.1791958171.gitgitgadget@gmail.com","subject":"[PATCH 3/5] wrapper: reject insanely large byte counts when reading/writing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-14T06:09:29Z","receivedAt":"2026-10-14T06:09:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBoth the `read_in_full()` as well as the `write_in_full()` function\naccumulate the bytes transferred count in a variable of type `ssize_t`,\nbut accept a `size_t` count parameter. When count exceeds `SSIZE_MAX`,\nthat total count overflows after processing `SSIZE_MAX` bytes, producing\na negative return value that callers interpret as an error (or worse, a\nsmall positive value after further wrapping).\n\nIn practice, git never passes such large counts because file sizes and\nbuffer allocations are bounded well below SSIZE_MAX on all target\nplatforms. The POSIX specification for `read()` and `write()` also\nlimits single operations to `SSIZE_MAX` bytes, and git's\n`xread()`/`xwrite()` wrappers further clamp to `MAX_IO_SIZE`. But the\naccumulation in the `*_in_full()` wrappers means that even with per-call\nclamping, total can exceed `SSIZE_MAX` when count does.\n\nReject a total count that exceeds the range of `ssize_t` upfront with\n`EINVAL` rather than silently producing wrong return values, matching\nthe POSIX convention for invalid arguments.\n\nPointed out by Coverity.\n\nAssisted-by: Claude Opus 4.6\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 4 ++++\n wrapper.c | 10 ++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a0f901ce79..afce8b85b8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -740,6 +740,10 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)\n */\n #define DEFAULT_IO_BUFFER_SIZE (128 * 1024)\n \n+#ifndef SSIZE_MAX\n+# define SSIZE_MAX ((ssize_t)(((size_t)-1) >> 1))\n+#endif\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\ndiff --git a/wrapper.c b/wrapper.c\nindex 05a9cd369c..ea9c14f834 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -288,6 +288,11 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \tchar *p = buf;\n \tssize_t total = 0;\n \n+\tif (count > SSIZE_MAX) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n \twhile (count > 0) {\n \t\tssize_t loaded = xread(fd, p, count);\n \t\tif (loaded < 0)\n@@ -307,6 +312,11 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \tconst char *p = buf;\n \tssize_t total = 0;\n \n+\tif (count > SSIZE_MAX) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n \twhile (count > 0) {\n \t\tssize_t written = xwrite(fd, p, count);\n \t\tif (written < 0)\n-- \ngitgitgadget\n\n\n"},{"id":"554948","messageId":"3dcc3d97570ea7f96628d02941d7424f84f7bf39.1791958171.git.gitgitgadget@gmail.com","threadId":"66527","inReplyTo":"pull.2253.git.1791958171.gitgitgadget@gmail.com","subject":"[PATCH 4/5] git-compat-util: add `signed_mult_overflows` and `mult_overflows` macros","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-14T06:09:30Z","receivedAt":"2026-10-14T06:09:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\n`unsigned_mult_overflows()` does not cover signed arithmetic, but types\nsuch as `time_t` can be signed or unsigned. Add signed and type-generic\nguards so callers can check multiplication overflow in either case.\n\nOverflow depends on the product type selected by C's usual arithmetic\nconversions. Products equal to the signed minimum are representable and\nmust not be rejected.\n\nAssisted-by: Claude Opus 4.6, GPT-6 and GPT-6.1\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 50 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex afce8b85b8..077fa94873 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -85,9 +85,27 @@ struct strbuf;\n \n #define bitsizeof(x) (CHAR_BIT * sizeof(x))\n \n+#if GIT_GNUC_PREREQ(3, 1)\n+#define is_unsigned_type(a) ((__typeof__(a))-1 >= 0)\n+#elif __STDC_VERSION__ >= 201112L\n+#define is_unsigned_type(a) _Generic((a), \\\n+\tunsigned char: 1, \\\n+\tunsigned short: 1, \\\n+\tunsigned int: 1, \\\n+\tunsigned long: 1, \\\n+\tunsigned long long: 1, \\\n+\t_Bool: 1, \\\n+\tchar: (CHAR_MIN == 0), \\\n+\tdefault: 0)\n+#endif\n+\n #define maximum_signed_value_of_type(a) \\\n (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))\n \n+/* Account for the extra negative value on two's-complement targets. */\n+#define minimum_signed_value_of_type(a) \\\n+ (-maximum_signed_value_of_type(a) - (INTMAX_MIN < -INTMAX_MAX))\n+\n #define maximum_unsigned_value_of_type(a) \\\n (UINTMAX_MAX >> (bitsizeof(uintmax_t) - bitsizeof(a)))\n \n@@ -111,6 +129,38 @@ struct strbuf;\n #define unsigned_mult_overflows(a, b) \\\n ((a) && (b) > maximum_unsigned_value_of_type(a) / (a))\n \n+/*\n+ * Returns true if the multiplication of \"a\" and \"b\" will\n+ * overflow or underflow a signed type. The types of \"a\" and \"b\"\n+ * must match and must be signed. Note that this macro evaluates\n+ * both \"a\" and \"b\" twice!\n+ */\n+#define signed_mult_overflows(a, b) \\\n+ (((a) > 0 && (b) > 0 && (a) > maximum_signed_value_of_type(a) / (b)) || \\\n+ ((a) < 0 && (b) < 0 && (a) < maximum_signed_value_of_type(a) / (b)) || \\\n+ ((a) > 0 && (b) < 0 && (b) < minimum_signed_value_of_type(a) / (a)) || \\\n+ ((a) < 0 && (b) > 0 && (a) < minimum_signed_value_of_type(b) / (b)))\n+\n+/*\n+ * Returns true if the product of \"a\" and \"b\", after the usual arithmetic\n+ * conversions, cannot be represented in the multiplication expression's\n+ * type. The operands must be integers and may be evaluated more than once.\n+ */\n+#if GIT_GNUC_PREREQ(3, 1)\n+#define mult_overflows(a, b) \\\n+ (is_unsigned_type((a) * (b)) \\\n+ ? unsigned_mult_overflows((__typeof__((a) * (b)))(a), \\\n+\t\t\t (__typeof__((a) * (b)))(b)) \\\n+ : signed_mult_overflows((__typeof__((a) * (b)))(a), \\\n+\t\t\t (__typeof__((a) * (b)))(b)))\n+#else\n+/*\n+ * This implementation needs GNU `typeof` to convert both operands to the\n+ * multiplication's type. Skip the check without that extension.\n+ */\n+#define mult_overflows(a, b) 0\n+#endif\n+\n /*\n * Returns true if the left shift of \"a\" by \"shift\" bits will\n * overflow. The type of \"a\" must be unsigned.\n-- \ngitgitgadget\n\n\n"},{"id":"554949","messageId":"a4e602b9e726081aaaf73c627548b823a4d35477.1791958171.git.gitgitgadget@gmail.com","threadId":"66527","inReplyTo":"pull.2253.git.1791958171.gitgitgadget@gmail.com","subject":"[PATCH 5/5] date: avoid overflow in approxidate relative-time multiplication","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-14T06:09:31Z","receivedAt":"2026-10-14T06:09:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `approxidate_alpha()` function multiplies a time unit length (e.g.,\n604800 for weeks) by a user-supplied number to compute a time offset in\nseconds. Both operands are `int`, and the product is passed to\n`update_tm()` as `time_t`. For inputs like \"99999 weeks ago\", the int\nmultiplication overflows (604800 * 99999 is roughly 60 billion, far\nbeyond `INT_MAX`), producing a wrapped value that results in a wrong\ndate.\n\nUse the checked `time_t` multiplication so oversized products do not\noverflow or wrap around.\n\nOn platforms where `time_t` is unsigned (and where the pre-processor\nallows us to determine this fact), reject counts that are too large and\nreport a parse error. Narrowing them first could otherwise produce\nnegative or truncated values that become bogus offsets. Leave signed\n`time_t` count parsing unchanged.\n\nPointed out by Coverity.\n\nAssisted-by: Claude Opus 4.6 and GPT-6.1 Sol\nSigned-off-by: Johannes Schindelin \n---\n date.c | 26 ++++++++++++++++++++++++--\n 1 file changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 014065b419..0d65804d2f 100644\n--- a/date.c\n+++ b/date.c\n@@ -1298,7 +1298,18 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \twhile (tl->type) {\n \t\tsize_t len = strlen(tl->type);\n \t\tif (match_string(date, tl->type) >= len-1) {\n-\t\t\tupdate_tm(tm, now, tl->length * *num);\n+\t\t\ttime_t length = tl->length, num_t = *num;\n+\t\t\ttime_t seconds;\n+#ifndef is_unsigned_type\n+\t\t\tseconds = length * num_t;\n+#else\n+\t\t\ttime_t max_val = is_unsigned_type(length)\n+\t\t\t\t? maximum_unsigned_value_of_type(length)\n+\t\t\t\t: maximum_signed_value_of_type(length);\n+\t\t\tseconds = mult_overflows(length, num_t)\n+\t\t\t\t? max_val : length * num_t;\n+#endif\n+\t\t\tupdate_tm(tm, now, seconds);\n \t\t\t*num = 0;\n \t\t\t*touched = 1;\n \t\t\treturn end;\n@@ -1368,8 +1379,15 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,\n \t}\n \n \t/* Accept zero-padding only for small numbers (\"Dec 02\", never \"Dec 0002\") */\n-\tif (date[0] != '0' || end - date <= 2)\n+\tif (date[0] != '0' || end - date <= 2) {\n+#ifdef is_unsigned_type\n+\t\tif (is_unsigned_type((time_t)0) &&\n+\t\t (number > INT_MAX ||\n+\t\t number > maximum_unsigned_value_of_type((time_t)0)))\n+\t\t\treturn NULL;\n+#endif\n \t\t*num = number;\n+\t}\n \treturn end;\n }\n \n@@ -1398,6 +1416,10 @@ static timestamp_t approxidate_str(const char *date,\n \t\tif (isdigit(c)) {\n \t\t\tpending_number(&tm, &number);\n \t\t\tdate = approxidate_digit(date-1, &tm, &number, time_sec);\n+\t\t\tif (!date) {\n+\t\t\t\t*error_ret = 1;\n+\t\t\t\treturn 0;\n+\t\t\t}\n \t\t\ttouched = 1;\n \t\t\tcontinue;\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"555012","messageId":"96c1d112-1b05-4663-90c6-33374dccd764@gmail.com","threadId":"66527","inReplyTo":"b1ed701935027a83574fae753e5cc11d23f0ea4b.1791958171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] wrapper: reject insanely large byte counts when reading/writing","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-10-14T11:37:59Z","receivedAt":"2026-10-14T11:37:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> > Both the `read_in_full()` as well as the `write_in_full()` function\n> accumulate the bytes transferred count in a variable of type `ssize_t`,\n> but accept a `size_t` count parameter. When count exceeds `SSIZE_MAX`,\n> that total count overflows after processing `SSIZE_MAX` bytes, producing\n> a negative return value that callers interpret as an error (or worse, a\n> small positive value after further wrapping).\n> > In practice, git never passes such large counts because file sizes and\n> buffer allocations are bounded well below SSIZE_MAX on all target\n> 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)\non 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.\n\nThanks\n\nPhillip\n\n> The POSIX specification for `read()` and `write()` also\n> limits single operations to `SSIZE_MAX` bytes, and git's\n> `xread()`/`xwrite()` wrappers further clamp to `MAX_IO_SIZE`. But the\n> accumulation in the `*_in_full()` wrappers means that even with per-call\n> clamping, total can exceed `SSIZE_MAX` when count does.\n> > Reject a total count that exceeds the range of `ssize_t` upfront with\n> `EINVAL` rather than silently producing wrong return values, matching\n> the POSIX convention for invalid arguments.\n> > Pointed out by Coverity.\n> > Assisted-by: Claude Opus 4.6\n> Signed-off-by: Johannes Schindelin \n> ---\n> git-compat-util.h | 4 ++++\n> wrapper.c | 10 ++++++++++\n> 2 files changed, 14 insertions(+)\n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> index a0f901ce79..afce8b85b8 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -740,6 +740,10 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b)\n> */\n> #define DEFAULT_IO_BUFFER_SIZE (128 * 1024)\n> > +#ifndef SSIZE_MAX\n> +# define SSIZE_MAX ((ssize_t)(((size_t)-1) >> 1))\n> +#endif\n> +\n> #ifdef HAVE_ALLOCA_H\n> # include \n> # define xalloca(size) (alloca(size))\n> diff --git a/wrapper.c b/wrapper.c\n> index 05a9cd369c..ea9c14f834 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -288,6 +288,11 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n> \tchar *p = buf;\n> \tssize_t total = 0;\n> > +\tif (count > SSIZE_MAX) {\n> +\t\terrno = EINVAL;\n> +\t\treturn -1;\n> +\t}\n> +\n> \twhile (count > 0) {\n> \t\tssize_t loaded = xread(fd, p, count);\n> \t\tif (loaded < 0)\n> @@ -307,6 +312,11 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n> \tconst char *p = buf;\n> \tssize_t total = 0;\n> > +\tif (count > SSIZE_MAX) {\n> +\t\terrno = EINVAL;\n> +\t\treturn -1;\n> +\t}\n> +\n> \twhile (count > 0) {\n> \t\tssize_t written = xwrite(fd, p, count);\n> \t\tif (written < 0)\n\n\n"},{"id":"555079","messageId":"xmqq4ieoq7n0.fsf@gitster.g","threadId":"66527","inReplyTo":"a4e602b9e726081aaaf73c627548b823a4d35477.1791958171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] date: avoid overflow in approxidate relative-time multiplication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-14T21:44:51Z","receivedAt":"2026-10-14T21:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> On platforms where `time_t` is unsigned (and where the pre-processor\n> allows us to determine this fact), reject counts that are too large and\n> report a parse error. Narrowing them first could otherwise produce\n> negative or truncated values that become bogus offsets. Leave signed\n> `time_t` count parsing unchanged.\n\nDoes this mean that \"git log -p\" in a project that were made on\nreasonably sized platforms and then cloned to my box may cause the\nhistory traversal to stop with an error when a commit has author or\ncommitter timestamp that would not fit the time_t on my box?\n\nIt would have been horrible regression than showing the entire\nhistory with patches with bogus timestamp if it were the case.\n\nBut I do not think that is what happens. approxidate_careful() is\nonly used to parse input by local user (e.g., \"git commit\n--date=\"...\", \"git log @{...}\"), and the changes in this patch would\nnot affect behaviours of other uses of timestamps, so we are safe.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> date.c | 26 ++++++++++++++++++++++++--\n> 1 file changed, 24 insertions(+), 2 deletions(-)\n>\n> diff --git a/date.c b/date.c\n> index 014065b419..0d65804d2f 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -1298,7 +1298,18 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n> \twhile (tl->type) {\n> \t\tsize_t len = strlen(tl->type);\n> \t\tif (match_string(date, tl->type) >= len-1) {\n> -\t\t\tupdate_tm(tm, now, tl->length * *num);\n> +\t\t\ttime_t length = tl->length, num_t = *num;\n> +\t\t\ttime_t seconds;\n> +#ifndef is_unsigned_type\n> +\t\t\tseconds = length * num_t;\n> +#else\n> +\t\t\ttime_t max_val = is_unsigned_type(length)\n> +\t\t\t\t? maximum_unsigned_value_of_type(length)\n> +\t\t\t\t: maximum_signed_value_of_type(length);\n> +\t\t\tseconds = mult_overflows(length, num_t)\n> +\t\t\t\t? max_val : length * num_t;\n> +#endif\n> +\t\t\tupdate_tm(tm, now, seconds);\n> \t\t\t*num = 0;\n> \t\t\t*touched = 1;\n> \t\t\treturn end;\n> @@ -1368,8 +1379,15 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,\n> \t}\n> \n> \t/* Accept zero-padding only for small numbers (\"Dec 02\", never \"Dec 0002\") */\n> -\tif (date[0] != '0' || end - date <= 2)\n> +\tif (date[0] != '0' || end - date <= 2) {\n> +#ifdef is_unsigned_type\n> +\t\tif (is_unsigned_type((time_t)0) &&\n> +\t\t (number > INT_MAX ||\n> +\t\t number > maximum_unsigned_value_of_type((time_t)0)))\n> +\t\t\treturn NULL;\n> +#endif\n> \t\t*num = number;\n> +\t}\n> \treturn end;\n> }\n> \n> @@ -1398,6 +1416,10 @@ static timestamp_t approxidate_str(const char *date,\n> \t\tif (isdigit(c)) {\n> \t\t\tpending_number(&tm, &number);\n> \t\t\tdate = approxidate_digit(date-1, &tm, &number, time_sec);\n> +\t\t\tif (!date) {\n> +\t\t\t\t*error_ret = 1;\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> \t\t\ttouched = 1;\n> \t\t\tcontinue;\n> \t\t}\n\n"},{"id":"555123","messageId":"373f7186-3da6-4715-a4ce-cd5f56895176@gmail.com","threadId":"66527","inReplyTo":"3dcc3d97570ea7f96628d02941d7424f84f7bf39.1791958171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] git-compat-util: add `signed_mult_overflows` and `mult_overflows` macros","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-10-15T09:27:42Z","receivedAt":"2026-10-15T09:27:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> > `unsigned_mult_overflows()` does not cover signed arithmetic, but types\n> such as `time_t` can be signed or unsigned. Add signed and type-generic\n> guards so callers can check multiplication overflow in either case.\n\nIt would have been nice to mention that the type-generic macro is gcc only so we cannot rely on this for safety checks.\n\n> #define maximum_signed_value_of_type(a) \\\n> (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))\n> > +/* Account for the extra negative value on two's-complement targets. */\n> +#define minimum_signed_value_of_type(a) \\\n> + (-maximum_signed_value_of_type(a) - (INTMAX_MIN < -INTMAX_MAX))\n\nDo 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.\n\n> +/*\n> + * Returns true if the multiplication of \"a\" and \"b\" will\n> + * overflow or underflow a signed type. The types of \"a\" and \"b\"\n> + * must match and must be signed. Note that this macro evaluates\n> + * both \"a\" and \"b\" twice!\n\nAt least twice I think\n\n> + */\n> +#define signed_mult_overflows(a, b) \\\n\nAs the macro relies on both operands being the same width, can we use BUILD_ASSERT_OR_ZERO() to ensure sizeof(a) == sizeof(b) here?\n\n> + (((a) > 0 && (b) > 0 && (a) > maximum_signed_value_of_type(a) / (b)) || \\\n> + ((a) < 0 && (b) < 0 && (a) < maximum_signed_value_of_type(a) / (b)) || \\\n> + ((a) > 0 && (b) < 0 && (b) < minimum_signed_value_of_type(a) / (a)) || \\\n> + ((a) < 0 && (b) > 0 && (a) < minimum_signed_value_of_type(b) / (b)))\n\nThanks\n\nPhillip\n\n> +\n> +/*\n> + * Returns true if the product of \"a\" and \"b\", after the usual arithmetic\n> + * conversions, cannot be represented in the multiplication expression's\n> + * type. The operands must be integers and may be evaluated more than once.\n> + */\n> +#if GIT_GNUC_PREREQ(3, 1)\n> +#define mult_overflows(a, b) \\\n> + (is_unsigned_type((a) * (b)) \\\n> + ? unsigned_mult_overflows((__typeof__((a) * (b)))(a), \\\n> +\t\t\t (__typeof__((a) * (b)))(b)) \\\n> + : signed_mult_overflows((__typeof__((a) * (b)))(a), \\\n> +\t\t\t (__typeof__((a) * (b)))(b)))\n> +#else\n> +/*\n> + * This implementation needs GNU `typeof` to convert both operands to the\n> + * multiplication's type. Skip the check without that extension.\n> + */\n> +#define mult_overflows(a, b) 0\n> +#endif\n> +\n> /*\n> * Returns true if the left shift of \"a\" by \"shift\" bits will\n> * overflow. The type of \"a\" must be unsigned.\n\n\n"},{"id":"555124","messageId":"163d14a8-711c-4d4f-b441-8268db819541@gmail.com","threadId":"66527","inReplyTo":"a4e602b9e726081aaaf73c627548b823a4d35477.1791958171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] date: avoid overflow in approxidate relative-time multiplication","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-10-15T09:28:15Z","receivedAt":"2026-10-15T09:28:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 14/10/2026 07:09, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> > The `approxidate_alpha()` function multiplies a time unit length (e.g.,\n> 604800 for weeks) by a user-supplied number to compute a time offset in\n> seconds. Both operands are `int`, and the product is passed to\n> `update_tm()` as `time_t`. For inputs like \"99999 weeks ago\", the int\n> multiplication overflows (604800 * 99999 is roughly 60 billion, far\n> beyond `INT_MAX`), producing a wrapped value that results in a wrong\n> date.\n\nSigned integer overflow is undefined so we don't necessarily get a wrapped value - the whole program is invalid.\n\nIt looks like \"length\" is always positive, can the user supplied number ever be negative?\n\n> Use the checked `time_t` multiplication so oversized products do not\n> overflow or wrap around.\n\nThis 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\n\n\tintmax_t length tl->length, num_t = *num, result;\n\n\tif (signed_mult_overflows(length, num_t))\n\t\t// error\n\tresult = length * num_t;\n\tif ((intmax_t)((time_t)result) != result ||\n\t (result < 0 && (time_t)result > 0))\n\t\t// error\n\nthat 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.\n\n> On platforms where `time_t` is unsigned (and where the pre-processor\n> allows us to determine this fact), reject counts that are too large and\n> report a parse error. Narrowing them first could otherwise produce\n> negative or truncated values that become bogus offsets. Leave signed\n> `time_t` count parsing unchanged.\n\nI'm a bit confused what this is trying to do - why can't we check this when time_t is signed?\n\n\tif ((timestamp_t)((int)number) != number)\n\t\treturn NULL\n\n\treturn *val = value;\n\nEnsures \"value\" fits in \"val\" without truncation, we could also add a check that value is positive when time_t is unsigned if we wanted.\n\nThanks\n\nPhillip\n> Pointed out by Coverity.\n> > Assisted-by: Claude Opus 4.6 and GPT-6.1 Sol\n> Signed-off-by: Johannes Schindelin \n> ---\n> date.c | 26 ++++++++++++++++++++++++--\n> 1 file changed, 24 insertions(+), 2 deletions(-)\n> > diff --git a/date.c b/date.c\n> index 014065b419..0d65804d2f 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -1298,7 +1298,18 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n> \twhile (tl->type) {\n> \t\tsize_t len = strlen(tl->type);\n> \t\tif (match_string(date, tl->type) >= len-1) {\n> -\t\t\tupdate_tm(tm, now, tl->length * *num);\n> +\t\t\ttime_t length = tl->length, num_t = *num;\n> +\t\t\ttime_t seconds;\n> +#ifndef is_unsigned_type\n> +\t\t\tseconds = length * num_t;\n> +#else\n> +\t\t\ttime_t max_val = is_unsigned_type(length)\n> +\t\t\t\t? maximum_unsigned_value_of_type(length)\n> +\t\t\t\t: maximum_signed_value_of_type(length);\n> +\t\t\tseconds = mult_overflows(length, num_t)\n> +\t\t\t\t? max_val : length * num_t;\n> +#endif\n> +\t\t\tupdate_tm(tm, now, seconds);\n> \t\t\t*num = 0;\n> \t\t\t*touched = 1;\n> \t\t\treturn end;\n> @@ -1368,8 +1379,15 @@ static const char *approxidate_digit(const char *date, struct tm *tm, int *num,\n> \t}\n> > \t/* Accept zero-padding only for small numbers (\"Dec 02\", never \"Dec 0002\") */\n> -\tif (date[0] != '0' || end - date <= 2)\n> +\tif (date[0] != '0' || end - date <= 2) {\n> +#ifdef is_unsigned_type\n> +\t\tif (is_unsigned_type((time_t)0) &&\n> +\t\t (number > INT_MAX ||\n> +\t\t number > maximum_unsigned_value_of_type((time_t)0)))\n> +\t\t\treturn NULL;\n> +#endif\n> \t\t*num = number;\n> +\t}\n> \treturn end;\n> }\n> > @@ -1398,6 +1416,10 @@ static timestamp_t approxidate_str(const char *date,\n> \t\tif (isdigit(c)) {\n> \t\t\tpending_number(&tm, &number);\n> \t\t\tdate = approxidate_digit(date-1, &tm, &number, time_sec);\n> +\t\t\tif (!date) {\n> +\t\t\t\t*error_ret = 1;\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> \t\t\ttouched = 1;\n> \t\t\tcontinue;\n> \t\t}\n\n\n"},{"id":"555222","messageId":"atHiiye4oi_lF5FP@pks.im","threadId":"66527","inReplyTo":"70a909de18e8cd8d282439fb51a9399baa8c7be8.1791958171.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-16T08:38:35Z","receivedAt":"2026-10-16T08:38:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 14, 2026 at 06:09:28AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/http.c b/http.c\n> index 4af3c29c76..809471f436 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -2848,7 +2848,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n> {\n> \tunsigned char expn[4096];\n> \tsize_t size = eltsize * nmemb;\n> -\tint posn = 0;\n> +\tsize_t posn = 0;\n> \tstruct http_object_request *freq = data;\n> \tstruct active_request_slot *slot = freq->slot;\n\nRight. This is how we access the variable:\n\n\tdo {\n\t\tssize_t retval = xwrite(freq->localfile,\n\t\t\t\t\t(char *) ptr + posn, size - posn);\n\t\tif (retval < 0)\n\t\t\treturn posn / eltsize;\n\t\tposn += retval;\n\t} while (posn < size);\n\nSo it's clear that it doesn't have to be signed either, as we only ever\nadd positive values to it.\n\nPatrick\n\n"},{"id":"555223","messageId":"CAHwyqnU3Y+kGOB7-E73dr53OV8dTcjNzZWU0jYSLf14=j4Ygmw@mail.gmail.com","threadId":"66527","inReplyTo":"pull.2253.git.1791958171.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Address overflows pointed out by Coverity","fromName":"Harald Nordgren","fromEmail":"haraldnordgren@gmail.com","sentAt":"2026-10-16T08:43:23Z","receivedAt":"2026-10-16T08:43:23Z","isPatch":true,"sender":{"key":"haraldnordgren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9569897?v=4"},"body":"> This patch series is part of my effort to bring Git for Windows' Coverity\n> reports down to zero; Previous patch series have been merged in 347fe8a91b5\n> (Merge branch 'js/coverity-fixes', 2026-07-16), 0b75ef5168c (Merge branch\n> 'js/coverity-fixes-null-safety', 2026-07-19), fcc8c931b53 (Merge branch\n> 'js/coverity-unchecked-returns-fix', 2026-08-24) and 8bc9c7fcfd8 (Merge\n> branch 'js/coverity-fixes', 2026-10-03).\n\nNice! Can Coverty run as part of GitHub CI? If so, once we have it\ndown to zero, we can enable it to stop issues from creeping back in\nover time.\n\n\nHarald\n\n"},{"id":"555240","messageId":"xmqq1p9pk99q.fsf@gitster.g","threadId":"66527","inReplyTo":"atHiiye4oi_lF5FP@pks.im","subject":"Re: [PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-16T14:30:41Z","receivedAt":"2026-10-16T14:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Oct 14, 2026 at 06:09:28AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>> diff --git a/http.c b/http.c\n>> index 4af3c29c76..809471f436 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -2848,7 +2848,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n>> {\n>> \tunsigned char expn[4096];\n>> \tsize_t size = eltsize * nmemb;\n>> -\tint posn = 0;\n>> +\tsize_t posn = 0;\n>> \tstruct http_object_request *freq = data;\n>> \tstruct active_request_slot *slot = freq->slot;\n>\n> Right. This is how we access the variable:\n>\n> \tdo {\n> \t\tssize_t retval = xwrite(freq->localfile,\n> \t\t\t\t\t(char *) ptr + posn, size - posn);\n> \t\tif (retval < 0)\n> \t\t\treturn posn / eltsize;\n> \t\tposn += retval;\n> \t} while (posn < size);\n>\n> So it's clear that it doesn't have to be signed either, as we only ever\n> add positive values to it.\n\nI agree that changing the type of posn from int to size_t is the\nright move.\n\nBut I wondered what happens when (size - posn) is larger than half\nthe range of the ssize_t type early in the loop. After all, the\nproposed commit log message worries about the case where HTTP\nresponse body exceeds 2GB.\n\nShouldn't the loop clamp the bytecount passed to xwrite() below\nwhatever would fit in ssize_t when successful? It may end up making\ntwo calls of xwrite, writing 1.5GB each, to write out 3GB payload,\nbut that would be better than attempting to write 3GB, succeed, and\nbe fooled by ssize_t that is narrower by size_t by at least one bit\nthat we got a negative return from xwrite() and return an error from\nhere, when in reality we have written them all?\n\n ... goes and looks ... \n\nAh, nevermind. The big comment in git-compat-util.h before\nMAX_IO_SIZE clarifies all of the above. In short, xwrite() takes\ncare of clamping, so its callers including this code do not have to\nworry about it.\n\n\n"},{"id":"555291","messageId":"395b47c0-6dc7-4a1b-b547-e5e68e7356f5@hogyros.de","threadId":"66527","inReplyTo":"xmqq1p9pk99q.fsf@gitster.g","subject":"Re: [PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file","fromName":"Simon Richter","fromEmail":"simon.richter@hogyros.de","sentAt":"2026-10-17T00:18:41Z","receivedAt":"2026-10-17T00:18:41Z","isPatch":true,"sender":{"key":"simon.richter@hogyros.de","avatar":"https://gravatar.com/avatar/1192aa9fa5dd19ce258b12b044cc27111dd7cdb58d920dc2123a02a24f55b5c5?d=mp&s=160"},"body":"Hi,\n\nOn 10/16/26 23:30, Junio C Hamano wrote:\n\n> I agree that changing the type of posn from int to size_t is the\n> right move.\n\nIt is a position in a file that is being read as binary data[1], so off_t would be the best choice I think.\n\nThat 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.\n\n Simon\n\n[1] to make a distinction from the use case for fpos_t\n"},{"id":"555296","messageId":"xmqqwlrharxf.fsf@gitster.g","threadId":"66527","inReplyTo":"395b47c0-6dc7-4a1b-b547-e5e68e7356f5@hogyros.de","subject":"Re: [PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-17T04:10:04Z","receivedAt":"2026-10-17T04:10:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Richter writes:\n\n>> I agree that changing the type of posn from int to size_t is the\n>> right move.\n>\n> It is a position in a file that is being read as binary data[1], so \n> off_t would be the best choice I think.\n\nHmm, interesting. The function writes out eltsize * nmemb (both are\nof type size_t) bytes, so the total could theoretically exceed the\nrange that fits size_t.\n\nBut the thing is that posn is used this way\n\n\t\tssize_t retval = xwrite(freq->localfile,\n\t\t\t\t\t(char *) ptr + posn, size - posn);\n\nto offset into a contiguous in-core region that starts at ptr. I am\nnot sure if (ptr + posn) is still valid for posn that is too large\nto fit in size_t, though.\n\nTo put it differently, we are writing out what is in the contiguous\nin-core region and posn runs up to the allocation size of that\nregion, which should be measured in size_t, no?\n\n"},{"id":"555297","messageId":"xmqqv771aruf.fsf@gitster.g","threadId":"66527","inReplyTo":"395b47c0-6dc7-4a1b-b547-e5e68e7356f5@hogyros.de","subject":"Re: [PATCH 2/5] http: use `size_t` for byte position in fwrite_sha1_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-17T04:11:52Z","receivedAt":"2026-10-17T04:11:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Richter writes:\n\n>> I agree that changing the type of posn from int to size_t is the\n>> right move.\n>\n> It is a position in a file that is being read as binary data[1], so \n> off_t would be the best choice I think.\n\nInteresting.\n\nThe function writes out eltsize * nmemb bytes (both of type\nsize_t), which means the product could theoretically exceed the\nrange of size_t. However, posn is used in xwrite() to offset\ninto a contiguous in-core buffer starting at ptr:\n\n ssize_t retval = xwrite(freq->localfile,\n (char *) ptr + posn,\n size - posn);\n\nBecause we are indexing into an allocated in-core region, any\nvalid offset must fit within size_t. If posn were to exceed\nthe bounds of size_t, the pointer arithmetic ptr + posn would\nalready be invalid. As posn only runs up to the allocation\nsize of the region, it indeed should be measured in size_t, no?\n\n\n"}]}