# [PATCH] REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed 10 messages from 2025-05-08 to 2025-05-09. Participants: Lidong Yan via GitGitGadget, René Scharfe, lidongyan, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/63420 ## Lidong Yan via GitGitGadget, 2025-05-08 13:39 Subject: [PATCH] REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed Message-ID: URL: https://gitlist.dev/e/pull.1955.git.git.1746711583166.gitgitgadget%40gmail.com ``` From: Lidong Yan <502024330056@smail.nju.edu.cn> REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc failed. This leak can be fixed by add a free(x) before set x to NULL. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> --- REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc failed. This leak can be fixed by add a free(x) before set x to NULL. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1955%2Fbrandb97%2Ffix-REFTABLE-REALLOC-leak-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1955/brandb97/fix-REFTABLE-REALLOC-leak-v1 Pull-Request: https://github.com/git/git/pull/1955 reftable/basics.h | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/reftable/basics.h b/reftable/basics.h index d8888c12629..c7651f0cda8 100644 --- a/reftable/basics.h +++ b/reftable/basics.h @@ -200,14 +200,26 @@ static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) } \ } while (0) #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ - size_t alloc_size; \ - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ - errno = ENOMEM; \ - (x) = NULL; \ - } else { \ - (x) = reftable_realloc((x), alloc_size); \ - } \ +#define REFTABLE_REALLOC_ARRAY(x, alloc) \ + do { \ + size_t alloc_size; \ + void *new_p; \ + if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < \ + 0) { \ + goto cleanup; \ + } else { \ + new_p = reftable_realloc((x), alloc_size); \ + if (!new_p) { \ + goto cleanup; \ + } \ + (x) = new_p; \ + } \ + break; \ + cleanup: \ + if (x) \ + free(x); \ + errno = ENOMEM; \ + (x) = NULL; \ } while (0) static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize, base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75 -- gitgitgadget ``` ## René Scharfe, 2025-05-08 21:42 Subject: Re: [PATCH] REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed Message-ID: <174cb568-e2bd-41e0-b090-eadd919d1ddd@web.de> URL: https://gitlist.dev/e/174cb568-e2bd-41e0-b090-eadd919d1ddd%40web.de In-Reply-To: ``` Am 08.05.25 um 15:39 schrieb Lidong Yan via GitGitGadget: > From: Lidong Yan <502024330056@smail.nju.edu.cn> > > REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc > failed. This leak can be fixed by add a free(x) before set x to NULL. Hmm, this macro is unused. Perhaps remove it? René ``` ## lidongyan, 2025-05-09 01:54 Subject: Re: [PATCH] REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed Message-ID: <4B1B3827-C617-4026-BE09-DBD91DCB5FF1@smail.nju.edu.cn> URL: https://gitlist.dev/e/4B1B3827-C617-4026-BE09-DBD91DCB5FF1%40smail.nju.edu.cn In-Reply-To: <174cb568-e2bd-41e0-b090-eadd919d1ddd@web.de> ``` Ok, I will remove it in the next patch. > 2025年5月9日 05:42，René Scharfe 写道： > > Am 08.05.25 um 15:39 schrieb Lidong Yan via GitGitGadget: >> From: Lidong Yan <502024330056@smail.nju.edu.cn> >> >> REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc >> failed. This leak can be fixed by add a free(x) before set x to NULL. > > Hmm, this macro is unused. Perhaps remove it? > > René > > ``` ## Lidong Yan via GitGitGadget, 2025-05-09 02:04 Subject: [PATCH v2] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro Message-ID: URL: https://gitlist.dev/e/pull.1955.v2.git.git.1746756263207.gitgitgadget%40gmail.com In-Reply-To: ``` From: Lidong Yan <502024330056@smail.nju.edu.cn> REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. Since it is unused, remove this unsafe macro. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> --- REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc failed. This leak can be fixed by add a free(x) before set x to NULL. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1955%2Fbrandb97%2Ffix-REFTABLE-REALLOC-leak-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1955/brandb97/fix-REFTABLE-REALLOC-leak-v2 Pull-Request: https://github.com/git/git/pull/1955 Range-diff vs v1: 1: 4d786a0ec17 ! 1: 6cc191f9db8 REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed @@ Metadata Author: Lidong Yan <502024330056@smail.nju.edu.cn> ## Commit message ## - REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed + REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro - REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc - failed. This leak can be fixed by add a free(x) before set x to NULL. + REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. + Since it is unused, remove this unsafe macro. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> ## reftable/basics.h ## @@ reftable/basics.h: static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) + (x) = reftable_malloc(alloc_size); \ } \ } while (0) - #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) +-#define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ - size_t alloc_size; \ - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ @@ reftable/basics.h: static inline int reftable_alloc_size(size_t nelem, size_t el - } else { \ - (x) = reftable_realloc((x), alloc_size); \ - } \ -+#define REFTABLE_REALLOC_ARRAY(x, alloc) \ -+ do { \ -+ size_t alloc_size; \ -+ void *new_p; \ -+ if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < \ -+ 0) { \ -+ goto cleanup; \ -+ } else { \ -+ new_p = reftable_realloc((x), alloc_size); \ -+ if (!new_p) { \ -+ goto cleanup; \ -+ } \ -+ (x) = new_p; \ -+ } \ -+ break; \ -+ cleanup: \ -+ if (x) \ -+ free(x); \ -+ errno = ENOMEM; \ -+ (x) = NULL; \ - } while (0) +- } while (0) ++#define REFTABLE_CALLOC_ARRAY(x, alloc) \ ++ (x) = reftable_calloc((alloc), sizeof(*(x))) static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize, + size_t *allocp) reftable/basics.h | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/reftable/basics.h b/reftable/basics.h index d8888c12629..667feffd935 100644 --- a/reftable/basics.h +++ b/reftable/basics.h @@ -199,16 +199,8 @@ static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) (x) = reftable_malloc(alloc_size); \ } \ } while (0) -#define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ - size_t alloc_size; \ - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ - errno = ENOMEM; \ - (x) = NULL; \ - } else { \ - (x) = reftable_realloc((x), alloc_size); \ - } \ - } while (0) +#define REFTABLE_CALLOC_ARRAY(x, alloc) \ + (x) = reftable_calloc((alloc), sizeof(*(x))) static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize, size_t *allocp) base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75 -- gitgitgadget ``` ## Junio C Hamano, 2025-05-09 03:43 Subject: Re: [PATCH] REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed Message-ID: URL: https://gitlist.dev/e/xmqqzffm30vs.fsf%40gitster.g In-Reply-To: <174cb568-e2bd-41e0-b090-eadd919d1ddd@web.de> ``` René Scharfe writes: > Am 08.05.25 um 15:39 schrieb Lidong Yan via GitGitGadget: >> From: Lidong Yan <502024330056@smail.nju.edu.cn> >> >> REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc >> failed. This leak can be fixed by add a free(x) before set x to NULL. > > Hmm, this macro is unused. Perhaps remove it? Or let the sleeping dog alone, perhaps? ``` ## Patrick Steinhardt, 2025-05-09 06:51 Subject: Re: [PATCH v2] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro Message-ID: URL: https://gitlist.dev/e/aB2l_WaBFyz5POld%40pks.im In-Reply-To: ``` On Fri, May 09, 2025 at 02:04:22AM +0000, Lidong Yan via GitGitGadget wrote: > diff --git a/reftable/basics.h b/reftable/basics.h > index d8888c12629..667feffd935 100644 > --- a/reftable/basics.h > +++ b/reftable/basics.h > @@ -199,16 +199,8 @@ static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) > (x) = reftable_malloc(alloc_size); \ > } \ > } while (0) > -#define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) > -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ > - size_t alloc_size; \ > - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ > - errno = ENOMEM; \ > - (x) = NULL; \ > - } else { \ > - (x) = reftable_realloc((x), alloc_size); \ > - } \ > - } while (0) > +#define REFTABLE_CALLOC_ARRAY(x, alloc) \ > + (x) = reftable_calloc((alloc), sizeof(*(x))) Let's avoid reformatting unrelated macros. But other than that I fully agree -- we should remove stuff that we don't use in the first place. Thanks! Patrick ``` ## lidongyan, 2025-05-09 07:38 Subject: Re: [PATCH v2] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro Message-ID: <02E0C9C6-3A1A-480C-834A-BB30C3170C78@smail.nju.edu.cn> URL: https://gitlist.dev/e/02E0C9C6-3A1A-480C-834A-BB30C3170C78%40smail.nju.edu.cn In-Reply-To: ``` 2025年5月9日 14:51，Patrick Steinhardt 写道： > > On Fri, May 09, 2025 at 02:04:22AM +0000, Lidong Yan via GitGitGadget wrote: >> diff --git a/reftable/basics.h b/reftable/basics.h >> index d8888c12629..667feffd935 100644 >> --- a/reftable/basics.h >> +++ b/reftable/basics.h >> @@ -199,16 +199,8 @@ static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) >> (x) = reftable_malloc(alloc_size); \ >> } \ >> } while (0) >> -#define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) >> -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ >> - size_t alloc_size; \ >> - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ >> - errno = ENOMEM; \ >> - (x) = NULL; \ >> - } else { \ >> - (x) = reftable_realloc((x), alloc_size); \ >> - } \ >> - } while (0) >> +#define REFTABLE_CALLOC_ARRAY(x, alloc) \ >> + (x) = reftable_calloc((alloc), sizeof(*(x))) > > Let's avoid reformatting unrelated macros. But other than that I fully > agree -- we should remove stuff that we don't use in the first place. > > Thanks! > > Patrick > Ok, I will restore REFTABLE_CALLOC_ARRAY in the next patch. ``` ## Lidong Yan via GitGitGadget, 2025-05-09 07:44 Subject: [PATCH v3] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro Message-ID: URL: https://gitlist.dev/e/pull.1955.v3.git.git.1746776686718.gitgitgadget%40gmail.com In-Reply-To: ``` From: Lidong Yan <502024330056@smail.nju.edu.cn> REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. Since it is unused, remove this unsafe macro. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> --- REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed REFTABLE_REALLOC_ARRAY doesn't free origin pointer when reftable_realloc failed. This leak can be fixed by add a free(x) before set x to NULL. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1955%2Fbrandb97%2Ffix-REFTABLE-REALLOC-leak-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1955/brandb97/fix-REFTABLE-REALLOC-leak-v3 Pull-Request: https://github.com/git/git/pull/1955 Range-diff vs v2: 1: 6cc191f9db8 ! 1: 107f9ce3bd0 REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro @@ Commit message ## reftable/basics.h ## @@ reftable/basics.h: static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) - (x) = reftable_malloc(alloc_size); \ } \ } while (0) --#define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) + #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ - size_t alloc_size; \ - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ @@ reftable/basics.h: static inline int reftable_alloc_size(size_t nelem, size_t el - (x) = reftable_realloc((x), alloc_size); \ - } \ - } while (0) -+#define REFTABLE_CALLOC_ARRAY(x, alloc) \ -+ (x) = reftable_calloc((alloc), sizeof(*(x))) static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize, size_t *allocp) reftable/basics.h | 9 --------- 1 file changed, 9 deletions(-) diff --git a/reftable/basics.h b/reftable/basics.h index d8888c12629..1674a6f8b80 100644 --- a/reftable/basics.h +++ b/reftable/basics.h @@ -200,15 +200,6 @@ static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) } \ } while (0) #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x))) -#define REFTABLE_REALLOC_ARRAY(x, alloc) do { \ - size_t alloc_size; \ - if (reftable_alloc_size(sizeof(*(x)), (alloc), &alloc_size) < 0) { \ - errno = ENOMEM; \ - (x) = NULL; \ - } else { \ - (x) = reftable_realloc((x), alloc_size); \ - } \ - } while (0) static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize, size_t *allocp) base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75 -- gitgitgadget ``` ## Patrick Steinhardt, 2025-05-09 08:23 Subject: Re: [PATCH v3] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro Message-ID: URL: https://gitlist.dev/e/aB27k8DI3BNgiwS-%40pks.im In-Reply-To: ``` On Fri, May 09, 2025 at 07:44:46AM +0000, Lidong Yan via GitGitGadget wrote: > From: Lidong Yan <502024330056@smail.nju.edu.cn> > > REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. > Since it is unused, remove this unsafe macro. This looks good to me, thanks! Patrick ``` ## Junio C Hamano, 2025-05-09 14:25 Subject: Re: [PATCH v3] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro Message-ID: URL: https://gitlist.dev/e/xmqqv7q93lor.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > On Fri, May 09, 2025 at 07:44:46AM +0000, Lidong Yan via GitGitGadget wrote: >> From: Lidong Yan <502024330056@smail.nju.edu.cn> >> >> REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. >> Since it is unused, remove this unsafe macro. > > This looks good to me, thanks! Alright. As long as we do not foresee adding a new caller of this in the near future, removal is always preferred ;-) Thanks. ```