REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed

PATCH In review Lidong Yan via GitGitGadget 5 participants 10 messages May 8, 2025 – May 9, 2025

lore

Share & tools

lore

Lidong Yan via GitGitGadget

author

2025-05-08 13:39 UTC #0 lore raw

PATCH 1/1 REFTABLE_REALLOC_ARRAY: fix potential memory leak if realloc failed

From: Lidong Yan <[email protected]>

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 <[email protected]>

---
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(-)

reftable/basics.h +20 -8

index d8888c12629..c7651f0cda8 100644
@@ -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

Signature · 1 line

gitgitgadget

René Scharfe

2025-05-08 21:42 UTC #1 lore raw

Am 08.05.25 um 15:39 schrieb Lidong Yan via GitGitGadget:

Quoted 4 lines

From: Lidong Yan <[email protected]>
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 UTC #2 lore raw

Ok, I will remove it in the next patch.

Quoted 13 lines

2025年5月9日 05:42，René Scharfe <[email protected]> 写道：
Am 08.05.25 um 15:39 schrieb Lidong Yan via GitGitGadget:
Quoted 4 lines
From: Lidong Yan <[email protected]>
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

author

2025-05-09 02:04 UTC #4 lore raw

PATCH v2 1/1 REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro

In reply to Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. Since it is unused, remove this unsafe macro.

Signed-off-by Lidong Yan <[email protected]>

---
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 <[email protected]>
      
       ## 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 <[email protected]>
      
       ## 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(-)

reftable/basics.h +2 ~~-10~~

index d8888c12629..667feffd935 100644
@@ -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

Signature · 1 line

gitgitgadget

Junio C Hamano

2025-05-09 03:43 UTC #3 lore raw

In reply to René Scharfe

Quoted 7 lines · René Scharfe

Am 08.05.25 um 15:39 schrieb Lidong Yan via GitGitGadget:
Quoted 4 lines
From: Lidong Yan <[email protected]>
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 UTC #5 lore raw

Re: [PATCH v2] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro

In reply to Lidong Yan via GitGitGadget

Quoted 10 lines · Lidong Yan via GitGitGadget

reftable/basics.h +0 -2

index d8888c12629..667feffd935 100644
@@ -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 { \

reftable/basics.h static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) · line 203

Quoted 10 lines

reftable/basics.h +2 -8

-		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 UTC #6 lore raw

Re: [PATCH v2] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro

Quoted 26 lines · 2025年5月9日 14:51，Patrick Steinhardt <[email protected]> 写道：

Quoted 10 lines · Lidong Yan via GitGitGadget
reftable/basics.h +0 -2
index d8888c12629..667feffd935 100644
@@ -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 { \
reftable/basics.h static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out) · line 203
Quoted 10 lines
reftable/basics.h +2 -8
- 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.

Quoted 4 lines

Thanks!
Patrick

Ok, I will restore REFTABLE_CALLOC_ARRAY in the next patch.

Lidong Yan via GitGitGadget

author

2025-05-09 07:44 UTC #7 lore raw

PATCH v3 1/1 REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro

In reply to Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>

REFTABLE_REALLOC_ARRAY will cause memory leak if realloc failed. Since it is unused, remove this unsafe macro.

Signed-off-by Lidong Yan <[email protected]>

---
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(-)

reftable/basics.h +0 -9

index d8888c12629..1674a6f8b80 100644
@@ -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

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2025-05-09 08:23 UTC #8 lore raw

Re: [PATCH v3] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro

Quoted 4 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
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:26 UTC #9 lore raw

Re: [PATCH v3] REFTABLE_REALLOC_ARRAY: remove this unsafe yet unused macro

Quoted 7 lines · Patrick Steinhardt

Quoted 4 lines · Lidong Yan via GitGitGadget
From: Lidong Yan <[email protected]>
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.