# [PATCH] last-modified: fix bug caused by inproper initialized memory 14 messages from 2025-11-28 to 2025-12-09. Participants: Toon Claes, Jeff King, Anders Kaseorg, Junio C Hamano. Thread: https://gitlist.dev/t/64546 ## Toon Claes, 2025-11-28 16:37 Subject: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <20251128-toon-big-endian-ci-v1-1-80da0f629c1e@iotcl.com> URL: https://gitlist.dev/e/20251128-toon-big-endian-ci-v1-1-80da0f629c1e%40iotcl.com ``` git-last-modified(1) uses a scratch bitmap to keep track of paths that have been changed between commits. To avoid reallocating a bitmap on each call of process_parent(), the scratch bitmap is kept and reused. Although, it seems an incorrect length is passed to memset(3). `struct bitmap` uses `eword_t` to for internal storage. This type is typedef'd to uint64_t. To fully zero the memory used by the bitmap, multiply the length (saved in `struct bitmap::word_alloc`) by the size of `eword_t`. Reported-by: Anders Kaseorg Helped-by: Jeff King Signed-off-by: Toon Claes --- It was reported [1] the tests in t8020 fail on s390x. After some research, it seems it was related to s390x being big-endian. Well, actually, not really. Using big-endian simply uncovered the problem in test. [1]: https://lore.kernel.org/git/4dc4c8cd-c0cc-4784-8fcf-defa3a051087@mit.edu/ --- builtin/last-modified.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/last-modified.c b/builtin/last-modified.c index b0ecbdc540..cc5fd2e795 100644 --- a/builtin/last-modified.c +++ b/builtin/last-modified.c @@ -327,7 +327,7 @@ static void process_parent(struct last_modified *lm, if (!(parent->object.flags & PARENT1)) active_paths_free(lm, parent); - memset(lm->scratch->words, 0x0, lm->scratch->word_alloc); + memset(lm->scratch->words, 0x0, lm->scratch->word_alloc * sizeof(eword_t)); diff_queue_clear(&diff_queued_diff); } --- base-commit: 6ab38b7e9cc7adafc304f3204616a4debd49c6e9 change-id: 20251126-toon-big-endian-ci-fe62bb361974 ``` ## Jeff King, 2025-11-28 20:55 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <20251128205514.GA605489@coredump.intra.peff.net> URL: https://gitlist.dev/e/20251128205514.GA605489%40coredump.intra.peff.net In-Reply-To: <20251128-toon-big-endian-ci-v1-1-80da0f629c1e@iotcl.com> ``` On Fri, Nov 28, 2025 at 05:37:13PM +0100, Toon Claes wrote: > git-last-modified(1) uses a scratch bitmap to keep track of paths that > have been changed between commits. To avoid reallocating a bitmap on > each call of process_parent(), the scratch bitmap is kept and reused. > Although, it seems an incorrect length is passed to memset(3). > > `struct bitmap` uses `eword_t` to for internal storage. This type is > typedef'd to uint64_t. To fully zero the memory used by the bitmap, > multiply the length (saved in `struct bitmap::word_alloc`) by the size > of `eword_t`. Good catch! When I was looking for casts that could be the culprit, I didn't think about the implicit one we get through the void pointer of memset(). > diff --git a/builtin/last-modified.c b/builtin/last-modified.c > index b0ecbdc540..cc5fd2e795 100644 > --- a/builtin/last-modified.c > +++ b/builtin/last-modified.c > @@ -327,7 +327,7 @@ static void process_parent(struct last_modified *lm, > if (!(parent->object.flags & PARENT1)) > active_paths_free(lm, parent); > > - memset(lm->scratch->words, 0x0, lm->scratch->word_alloc); > + memset(lm->scratch->words, 0x0, lm->scratch->word_alloc * sizeof(eword_t)); > diff_queue_clear(&diff_queued_diff); > } I think this patch makes sense as the most obvious and immediate fix. But thinking on how we might have avoided this bug: - We have macros like ALLOC_ARRAY() and COPY_ARRAY() that automatically multiply the array length by the size of each element (by looking at the type of the array). We could in theory have a helper like: MEMSET_ARRAY(lm->scratch->words, 0x0, lm->scratch->word_alloc); that would have made this hard to get wrong. But that's actually a bit of a funny interface, because memset is inherently byte-oriented under the hood. So we are not setting each element to 0x0, but rather each byte. For a value of 0x0, that is the same thing. But if you chose, say "0x1", it is not. So it would probably have to be limited to something like: CLEAR_ARRAY(lm->scratch->words, lm->scratch->word_alloc); which I'd guess would cover most memset cases. But this is getting specific enough that maybe the macro is making things more confusing rather than less. - It's a little gross that we are reaching inside a "struct bitmap" in the first place, as it's a mostly opaque type. And the code here has to know that the alloc field is sized in eword_t's, not in bytes. It feels like there should be a bitmap_clear() function. Its implementation would also have to remember to multiply by sizeof(eword_t), but at least it would be encapsulated. I doubt the leaky abstraction matters that much, though. It seems unlikely that we would change it (and if we did, we'd perhaps give the field a new name). In the same vein, probably using "sizeof(lm->scratch->words)" is better than "sizeof(eword_t)". But again, I find it an unlikely detail for us to catch under the hood. -Peff ``` ## Anders Kaseorg, 2025-11-28 22:20 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <5699f2cc-5157-441e-af98-4d8df492ec72@mit.edu> URL: https://gitlist.dev/e/5699f2cc-5157-441e-af98-4d8df492ec72%40mit.edu In-Reply-To: <20251128205514.GA605489@coredump.intra.peff.net> ``` On 11/28/25 12:55, Jeff King wrote: > In the same vein, probably using "sizeof(lm->scratch->words)" is > better than "sizeof(eword_t)". But again, I find it an unlikely > detail for us to catch under the hood. As words is a pointer, you must have meant sizeof *lm->scratch->words or sizeof lm->scratch->words[0]. Anders ``` ## Junio C Hamano, 2025-11-29 02:01 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: URL: https://gitlist.dev/e/xmqq8qfpioln.fsf%40gitster.g In-Reply-To: <20251128-toon-big-endian-ci-v1-1-80da0f629c1e@iotcl.com> ``` Toon Claes writes: > git-last-modified(1) uses a scratch bitmap to keep track of paths that > have been changed between commits. To avoid reallocating a bitmap on > each call of process_parent(), the scratch bitmap is kept and reused. > Although, it seems an incorrect length is passed to memset(3). > > `struct bitmap` uses `eword_t` to for internal storage. This type is > typedef'd to uint64_t. To fully zero the memory used by the bitmap, > multiply the length (saved in `struct bitmap::word_alloc`) by the size > of `eword_t`. > > Reported-by: Anders Kaseorg > Helped-by: Jeff King > Signed-off-by: Toon Claes > --- > It was reported [1] the tests in t8020 fail on s390x. After some > research, it seems it was related to s390x being big-endian. Well, > actually, not really. Using big-endian simply uncovered the problem in > test. > > [1]: https://lore.kernel.org/git/4dc4c8cd-c0cc-4784-8fcf-defa3a051087@mit.edu/ > --- > builtin/last-modified.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This dates back to v2.52.0~4 and is clearly a maint material. Thanks for finding and fixing. > > diff --git a/builtin/last-modified.c b/builtin/last-modified.c > index b0ecbdc540..cc5fd2e795 100644 > --- a/builtin/last-modified.c > +++ b/builtin/last-modified.c > @@ -327,7 +327,7 @@ static void process_parent(struct last_modified *lm, > if (!(parent->object.flags & PARENT1)) > active_paths_free(lm, parent); > > - memset(lm->scratch->words, 0x0, lm->scratch->word_alloc); > + memset(lm->scratch->words, 0x0, lm->scratch->word_alloc * sizeof(eword_t)); > diff_queue_clear(&diff_queued_diff); > } > > > --- > base-commit: 6ab38b7e9cc7adafc304f3204616a4debd49c6e9 > change-id: 20251126-toon-big-endian-ci-fe62bb361974 ``` ## Junio C Hamano, 2025-11-29 02:11 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: URL: https://gitlist.dev/e/xmqqwm39h9kb.fsf%40gitster.g In-Reply-To: ``` Junio C Hamano writes: > This dates back to v2.52.0~4 and is clearly a maint material. > > Thanks for finding and fixing. > Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Let's retitle, as inproper is not a word. Is Subject: [PATCH] last-modified: fix use of uninitialized memory good enough? ``` ## Toon Claes, 2025-11-29 09:38 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <87ms4518n1.fsf@iotcl.com> URL: https://gitlist.dev/e/87ms4518n1.fsf%40iotcl.com In-Reply-To: ``` Junio C Hamano writes: > Junio C Hamano writes: > >> This dates back to v2.52.0~4 and is clearly a maint material. Makes sense. I appreciate it. >> Thanks for finding and fixing. Yes, I'm happy Anders reported this, although I didn't expect it to have impact on all platforms. It would have been a nasty bug to hunt down if users would complain "the results are incorrect". >> Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory > > Let's retitle, as inproper is not a word. Is I wasn't sure about that. But my spell checker didn't pick it up, so I rolled with it. > Subject: [PATCH] last-modified: fix use of uninitialized memory > > good enough? Absolutely. -- Cheers, Toon ``` ## Jeff King, 2025-11-29 10:50 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <20251129105023.GA646133@coredump.intra.peff.net> URL: https://gitlist.dev/e/20251129105023.GA646133%40coredump.intra.peff.net In-Reply-To: <5699f2cc-5157-441e-af98-4d8df492ec72@mit.edu> ``` On Fri, Nov 28, 2025 at 02:20:22PM -0800, Anders Kaseorg wrote: > On 11/28/25 12:55, Jeff King wrote: > > In the same vein, probably using "sizeof(lm->scratch->words)" is better > > than "sizeof(eword_t)". But again, I find it an unlikely detail for us > > to catch under the hood. > > As words is a pointer, you must have meant sizeof *lm->scratch->words or > sizeof lm->scratch->words[0]. Whoops, yes. I prefer sizeof(*var) over sizeof(type) because it tracks changes to the type of "var" automatically. But the opportunity to forget the "*" is perhaps a point against it. :) -Peff ``` ## Toon Claes, 2025-12-08 11:46 Subject: [PATCH v2] last-modified: fix use of uninitialized memory Message-ID: <20251208-toon-big-endian-ci-v2-1-76b46763a597@iotcl.com> URL: https://gitlist.dev/e/20251208-toon-big-endian-ci-v2-1-76b46763a597%40iotcl.com In-Reply-To: <20251128-toon-big-endian-ci-v1-1-80da0f629c1e@iotcl.com> ``` git-last-modified(1) uses a scratch bitmap to keep track of paths that have been changed between commits. To avoid reallocating a bitmap on each call of process_parent(), the scratch bitmap is kept and reused. Although, between loops, the memory allocated for the 'scratch' bitmap isn't correctly wiped. `struct bitmap` uses `eword_t` to for internal storage. This type is typedef'd to uint64_t. To fully zero the memory used by the bitmap, the length (saved in `struct bitmap::word_alloc`) should be multiplied by the size of a single item. To simplify zeroing an array, a macro MEMZERO_ARRAY() is defined and used. Reported-by: Anders Kaseorg Helped-by: Jeff King Signed-off-by: Toon Claes --- It was reported [1] the tests in t8020 fail on s390x. After some research, it seems it was related to s390x being big-endian. Well, actually, not really. Using big-endian simply uncovered the problem in test. [1]: https://lore.kernel.org/git/4dc4c8cd-c0cc-4784-8fcf-defa3a051087@mit.edu/ --- Changes in v2: - Defined and used MEMZERO_ARRAY() macro. - Fixed up title which used unexisting word - Link to v1: https://lore.kernel.org/r/20251128-toon-big-endian-ci-v1-1-80da0f629c1e@iotcl.com --- builtin/last-modified.c | 2 +- git-compat-util.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/last-modified.c b/builtin/last-modified.c index b0ecbdc540..ac5387e861 100644 --- a/builtin/last-modified.c +++ b/builtin/last-modified.c @@ -327,7 +327,7 @@ static void process_parent(struct last_modified *lm, if (!(parent->object.flags & PARENT1)) active_paths_free(lm, parent); - memset(lm->scratch->words, 0x0, lm->scratch->word_alloc); + MEMZERO_ARRAY(lm->scratch->words, lm->scratch->word_alloc); diff_queue_clear(&diff_queued_diff); } diff --git a/git-compat-util.h b/git-compat-util.h index 398e0fac4f..2b8192fd2e 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -726,6 +726,7 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b) #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc))) #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x))) #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc))) +#define MEMZERO_ARRAY(x, alloc) memset((x), 0x0, st_mult(sizeof(*(x)), (alloc))) #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \ BARF_UNLESS_COPYABLE((dst), (src))) --- base-commit: bdc5341ff65278a3cc80b2e8a02a2f02aa1fac06 change-id: 20251126-toon-big-endian-ci-fe62bb361974 ``` ## Toon Claes, 2025-12-08 11:47 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <87bjk9w5yv.fsf@iotcl.com> URL: https://gitlist.dev/e/87bjk9w5yv.fsf%40iotcl.com In-Reply-To: <20251128205514.GA605489@coredump.intra.peff.net> ``` Jeff King writes: > I think this patch makes sense as the most obvious and immediate fix. > But thinking on how we might have avoided this bug: > > - We have macros like ALLOC_ARRAY() and COPY_ARRAY() that > automatically multiply the array length by the size of each element > (by looking at the type of the array). We could in theory have a > helper like: > > MEMSET_ARRAY(lm->scratch->words, 0x0, lm->scratch->word_alloc); > > that would have made this hard to get wrong. But that's actually a > bit of a funny interface, because memset is inherently byte-oriented > under the hood. So we are not setting each element to 0x0, but > rather each byte. For a value of 0x0, that is the same thing. But if > you chose, say "0x1", it is not. > > So it would probably have to be limited to something like: > > CLEAR_ARRAY(lm->scratch->words, lm->scratch->word_alloc); > > which I'd guess would cover most memset cases. But this is getting > specific enough that maybe the macro is making things more confusing > rather than less. I've submitted a v2 that introduces MEMZERO_ARRAY(). I'm curious what the responses on this proposal are? -- Cheers, Toon ``` ## Junio C Hamano, 2025-12-08 13:26 Subject: Re: [PATCH v2] last-modified: fix use of uninitialized memory Message-ID: URL: https://gitlist.dev/e/xmqqikehkstt.fsf%40gitster.g In-Reply-To: <20251208-toon-big-endian-ci-v2-1-76b46763a597@iotcl.com> ``` Toon Claes writes: > Changes in v2: > - Defined and used MEMZERO_ARRAY() macro. > - Fixed up title which used unexisting word > - Link to v1: https://lore.kernel.org/r/20251128-toon-big-endian-ci-v1-1-80da0f629c1e@iotcl.com Sorry, but hasn't the old one already been cooking in 'next'? > --- > builtin/last-modified.c | 2 +- > git-compat-util.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/builtin/last-modified.c b/builtin/last-modified.c > index b0ecbdc540..ac5387e861 100644 > --- a/builtin/last-modified.c > +++ b/builtin/last-modified.c > @@ -327,7 +327,7 @@ static void process_parent(struct last_modified *lm, > if (!(parent->object.flags & PARENT1)) > active_paths_free(lm, parent); > > - memset(lm->scratch->words, 0x0, lm->scratch->word_alloc); > + MEMZERO_ARRAY(lm->scratch->words, lm->scratch->word_alloc); > diff_queue_clear(&diff_queued_diff); > } > > diff --git a/git-compat-util.h b/git-compat-util.h > index 398e0fac4f..2b8192fd2e 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -726,6 +726,7 @@ static inline uint64_t u64_add(uint64_t a, uint64_t b) > #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc))) > #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x))) > #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc))) > +#define MEMZERO_ARRAY(x, alloc) memset((x), 0x0, st_mult(sizeof(*(x)), (alloc))) > > #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \ > BARF_UNLESS_COPYABLE((dst), (src))) > > --- > base-commit: bdc5341ff65278a3cc80b2e8a02a2f02aa1fac06 > change-id: 20251126-toon-big-endian-ci-fe62bb361974 ``` ## Jeff King, 2025-12-08 20:15 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: <20251208201501.GA216526@coredump.intra.peff.net> URL: https://gitlist.dev/e/20251208201501.GA216526%40coredump.intra.peff.net In-Reply-To: <87bjk9w5yv.fsf@iotcl.com> ``` On Mon, Dec 08, 2025 at 12:47:20PM +0100, Toon Claes wrote: > > So it would probably have to be limited to something like: > > > > CLEAR_ARRAY(lm->scratch->words, lm->scratch->word_alloc); > > > > which I'd guess would cover most memset cases. But this is getting > > specific enough that maybe the macro is making things more confusing > > rather than less. > > I've submitted a v2 that introduces MEMZERO_ARRAY(). I'm curious what > the responses on this proposal are? I think it looks fine, though as Junio noted, the original is already in next so it would have to be a patch on top. Is such a macro worth it? I guess we'd be able to see if there are other possible sites with something like: git grep 'memset(.*0,.*\* \?sizeof' that's looking for memsets of "0" that also multiply by sizeof. Looks like there are a few: add-patch.c: memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk)); builtin/last-modified.c: memset(lm->scratch->words, 0x0, lm->scratch->word_alloc * sizeof(eword_t)); compat/simple-ipc/ipc-win32.c: memset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS)); diff-delta.c: memset(hash, 0, hsize * sizeof(*hash)); hashmap.c: memset(map->table, 0, map->tablesize * sizeof(struct hashmap_entry *)); pack-revindex.c: memset(pos, 0, BUCKETS * sizeof(*pos)); The first one is an oddball, but the other five could use it. So if we were to do a patch adding MEMZERO_ARRAY(), it would probably make sense to convert those spots. I'd be OK either way. -Peff ``` ## Junio C Hamano, 2025-12-08 22:42 Subject: Re: [PATCH] last-modified: fix bug caused by inproper initialized memory Message-ID: URL: https://gitlist.dev/e/xmqq5xaglhn3.fsf%40gitster.g In-Reply-To: <20251208201501.GA216526@coredump.intra.peff.net> ``` Jeff King writes: > git grep 'memset(.*0,.*\* \?sizeof' > > that's looking for memsets of "0" that also multiply by sizeof. Looks > like there are a few: > > add-patch.c: memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk)); > builtin/last-modified.c: memset(lm->scratch->words, 0x0, lm->scratch->word_alloc * sizeof(eword_t)); > compat/simple-ipc/ipc-win32.c: memset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS)); > diff-delta.c: memset(hash, 0, hsize * sizeof(*hash)); > hashmap.c: memset(map->table, 0, map->tablesize * sizeof(struct hashmap_entry *)); > pack-revindex.c: memset(pos, 0, BUCKETS * sizeof(*pos)); > > The first one is an oddball, but the other five could use it. So if we > were to do a patch adding MEMZERO_ARRAY(), it would probably make sense > to convert those spots. I'd be OK either way. Thanks for making an excellent suggestion while I was away from the keyboard ;-) Between MEMZERO_ARRAY() and CLEAR_ARRAY(), I am on the fence. ``` ## Toon Claes, 2025-12-09 08:43 Subject: Re: [PATCH v2] last-modified: fix use of uninitialized memory Message-ID: <871pl4vyd5.fsf@iotcl.com> URL: https://gitlist.dev/e/871pl4vyd5.fsf%40iotcl.com In-Reply-To: ``` Junio C Hamano writes: > Sorry, but hasn't the old one already been cooking in 'next'? Okay, fine by me. Let's abandon this v2 then. -- Cheers, Toon ``` ## Junio C Hamano, 2025-12-09 12:18 Subject: Re: [PATCH v2] last-modified: fix use of uninitialized memory Message-ID: URL: https://gitlist.dev/e/xmqqpl8nkfwh.fsf%40gitster.g In-Reply-To: <871pl4vyd5.fsf@iotcl.com> ``` Toon Claes writes: > Junio C Hamano writes: > >> Sorry, but hasn't the old one already been cooking in 'next'? > > Okay, fine by me. Let's abandon this v2 then. Understood. I however agree with Patrick that rewriting memset(ptr, '\0', sizeof(*ptr) * nr) to use CLEAR_ARRAY(), not limited to last-modified but everywhere in the codebase, may not be a bad idea. It would be a good exercise to hone our Coccinelle skill ;-) ```