[05/11] submodule-config: avoid memory leak

Commit Message

Johannes Schindelin June 15, 2022, 11:35 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In 961b130d20c9 (branch: add --recurse-submodules option for branch
creation, 2022-01-28), a funny pattern was introduced where first some
struct is `xmalloc()`ed, then we resize an array whose element type is
the same struct, and then the first struct's contents are copied into
the last element of that array.

Crucially, the `xmalloc()`ed memory never gets released.

Let's avoid that memory leak and that memory allocation dance altogether
by first reallocating the array, then using a pointer to the last array
element to go forward.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 submodule-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Junio C Hamano June 16, 2022, 4:36 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> In 961b130d20c9 (branch: add --recurse-submodules option for branch
> creation, 2022-01-28), a funny pattern was introduced where first some
> struct is `xmalloc()`ed, then we resize an array whose element type is
> the same struct, and then the first struct's contents are copied into
> the last element of that array.

Sigh.  The original is butt ugly, with this strange pattern and
structure assignments etc.  I wonder how something like this slipped
through our reviews.

I wonder if it would help for me to stop trusting reviews by less
experienced reviewers too much, and instead give sanity checks to
more patches myself from now on, but I certainly cannot afford the
time and my mental health to do so for all the patches X-<.

Will queue.

>  		if (S_ISGITLINK(name_entry->mode) &&
>  		    is_tree_submodule_active(r, root_tree, tree_path)) {
> -			st_entry = xmalloc(sizeof(*st_entry));
> +			ALLOC_GROW(out->entries, out->entry_nr + 1,
> +				   out->entry_alloc);
> +			st_entry = &out->entries[out->entry_nr++];
> +
>  			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
>  			*st_entry->name_entry = *name_entry;
>  			st_entry->submodule =
> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
>  						root_tree))
>  				FREE_AND_NULL(st_entry->repo);
>  
> -			ALLOC_GROW(out->entries, out->entry_nr + 1,
> -				   out->entry_alloc);
> -			out->entries[out->entry_nr++] = *st_entry;
>  		} else if (S_ISDIR(name_entry->mode))
>  			traverse_tree_submodules(r, root_tree, tree_path,
>  						 &name_entry->oid, out);

Glen Choo June 16, 2022, 6:09 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
>> From: Johannes Schindelin <[e-mail]>
>>
>> In 961b130d20c9 (branch: add --recurse-submodules option for branch
>> creation, 2022-01-28), a funny pattern was introduced where first some
>> struct is `xmalloc()`ed, then we resize an array whose element type is
>> the same struct, and then the first struct's contents are copied into
>> the last element of that array.
>
> Sigh.  The original is butt ugly, with this strange pattern and
> structure assignments etc.  I wonder how something like this slipped
> through our reviews.

Gah, I have no excuses for bad code I've already written, I can only
strive to write better code next time.

Thanks Johannes for spotting and fixing this.

> I wonder if it would help for me to stop trusting reviews by less
> experienced reviewers too much, and instead give sanity checks to
> more patches myself from now on, but I certainly cannot afford the
> time and my mental health to do so for all the patches X-<.

I seem to recall that this was reviewed by fairly experienced folks. I'm
guessing it slipped through due to reviewer fatigue, which might be a
good argument for adding more tooling to lighten reviewer load/patch up
the gaps.

>
> Will queue.
>
>>  		if (S_ISGITLINK(name_entry->mode) &&
>>  		    is_tree_submodule_active(r, root_tree, tree_path)) {
>> -			st_entry = xmalloc(sizeof(*st_entry));
>> +			ALLOC_GROW(out->entries, out->entry_nr + 1,
>> +				   out->entry_alloc);
>> +			st_entry = &out->entries[out->entry_nr++];
>> +
>>  			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
>>  			*st_entry->name_entry = *name_entry;
>>  			st_entry->submodule =
>> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
>>  						root_tree))
>>  				FREE_AND_NULL(st_entry->repo);
>>  
>> -			ALLOC_GROW(out->entries, out->entry_nr + 1,
>> -				   out->entry_alloc);
>> -			out->entries[out->entry_nr++] = *st_entry;
>>  		} else if (S_ISDIR(name_entry->mode))
>>  			traverse_tree_submodules(r, root_tree, tree_path,
>>  						 &name_entry->oid, out);

Patch

diff --git a/submodule-config.c b/submodule-config.c
index ce3beaf5d4f..51ecbe901ec 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -756,7 +756,10 @@  static void traverse_tree_submodules(struct repository *r,
 
 		if (S_ISGITLINK(name_entry->mode) &&
 		    is_tree_submodule_active(r, root_tree, tree_path)) {
-			st_entry = xmalloc(sizeof(*st_entry));
+			ALLOC_GROW(out->entries, out->entry_nr + 1,
+				   out->entry_alloc);
+			st_entry = &out->entries[out->entry_nr++];
+
 			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
 			*st_entry->name_entry = *name_entry;
 			st_entry->submodule =
@@ -766,9 +769,6 @@  static void traverse_tree_submodules(struct repository *r,
 						root_tree))
 				FREE_AND_NULL(st_entry->repo);
 
-			ALLOC_GROW(out->entries, out->entry_nr + 1,
-				   out->entry_alloc);
-			out->entries[out->entry_nr++] = *st_entry;
 		} else if (S_ISDIR(name_entry->mode))
 			traverse_tree_submodules(r, root_tree, tree_path,
 						 &name_entry->oid, out);