diff mbox series

[05/11] submodule--helper: free() leaking {run,capture}_command() argument

Message ID patch-05.11-76eab92c8b6-20220713T131601Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series submodule--helper: fix memory leaks | expand

Commit Message

Ævar Arnfjörð Bjarmason July 13, 2022, 1:16 p.m. UTC 
Free the "dir" member of "struct child_process" that various functions
in submodule-helper.c allocate allocates with xstrdup().

Since the "dir" argument is "const char *" let's keep a
"char *to_free" variable around for this rather than casting when we
call free().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 41 +++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 11 deletions(-)

Comments

Glen Choo July 14, 2022, 3:36 a.m. UTC | #1 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Free the "dir" member of "struct child_process" that various functions
> in submodule-helper.c allocate allocates with xstrdup().
>
> Since the "dir" argument is "const char *" let's keep a
> "char *to_free" variable around for this rather than casting when we
> call free().
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 41 +++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a8e439e59b8..2099c5774b2 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2198,27 +2198,36 @@ static int is_tip_reachable(const char *path, struct object_id *oid)
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf rev = STRBUF_INIT;
>  	char *hex = oid_to_hex(oid);
> +	char *to_free;
> +	int ret;
>  
>  	cp.git_cmd = 1;
> -	cp.dir = xstrdup(path);
> +	cp.dir = to_free = xstrdup(path);
>  	cp.no_stderr = 1;
>  	strvec_pushl(&cp.args, "rev-list", "-n", "1", hex, "--not", "--all", NULL);
>  
>  	prepare_submodule_repo_env(&cp.env);
>  
> -	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
> -		return 0;
> +	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len) {
> +		ret = 0;
> +		goto cleanup;
> +	}
>  
> -	return 1;
> +	ret = 1;
> +cleanup:
> +	free(to_free);
> +	return ret;
>  }
>  
>  static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> +	char *to_free;
> +	int ret;
>  
>  	prepare_submodule_repo_env(&cp.env);
>  	cp.git_cmd = 1;
> -	cp.dir = xstrdup(module_path);
> +	cp.dir = to_free = xstrdup(module_path);
>  
>  	strvec_push(&cp.args, "fetch");
>  	if (quiet)
> @@ -2232,7 +2241,9 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
>  		free(remote);
>  	}
>  
> -	return run_command(&cp);
> +	ret = run_command(&cp);
> +	free(to_free);
> +	return ret;
>  }
>  
>  static int run_update_command(struct update_data *ud, int subforce)
> @@ -2240,6 +2251,8 @@ static int run_update_command(struct update_data *ud, int subforce)
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	char *oid = oid_to_hex(&ud->oid);
>  	int must_die_on_failure = 0;
> +	char *to_free;
> +	int ret;
>  
>  	switch (ud->update_strategy.type) {
>  	case SM_UPDATE_CHECKOUT:
> @@ -2273,7 +2286,7 @@ static int run_update_command(struct update_data *ud, int subforce)
>  	}
>  	strvec_push(&cp.args, oid);
>  
> -	cp.dir = xstrdup(ud->sm_path);
> +	cp.dir = to_free = xstrdup(ud->sm_path);
>  	prepare_submodule_repo_env(&cp.env);
>  	if (run_command(&cp)) {
>  		switch (ud->update_strategy.type) {
> @@ -2301,11 +2314,14 @@ static int run_update_command(struct update_data *ud, int subforce)
>  			exit(128);
>  
>  		/* the command failed, but update must continue */
> -		return 1;
> +		ret = 1;
> +		goto cleanup;
>  	}
>  
> -	if (ud->quiet)
> -		return 0;
> +	if (ud->quiet) {
> +		ret = 0;
> +		goto cleanup;
> +	}
>  
>  	switch (ud->update_strategy.type) {
>  	case SM_UPDATE_CHECKOUT:
> @@ -2329,7 +2345,10 @@ static int run_update_command(struct update_data *ud, int subforce)
>  		    submodule_strategy_to_string(&ud->update_strategy));
>  	}
>  
> -	return 0;
> +	ret = 0;
> +cleanup:
> +	free(to_free);
> +	return ret;
>  }
>  
>  static int run_update_procedure(struct update_data *ud)

I assume I'm missing something, but couldn't we achieve the same result
by just removing the xstrdup() calls? i.e. we only leak .dir (which we
don't clear because it's const), so couldn't we just assign to it
without xstrdup() and not have to worry about free()-ing it?

I didn't see a correctness reason for us to xstrdup(), and indeed, t7406
passes with the change I just described (which I believe covers all of
the sites here). In fact, we already have one site that does exactly
this in the recursive part of update_submodule():

	if (update_data->recursive) {
		struct child_process cp = CHILD_PROCESS_INIT;
		struct update_data next = *update_data;
		int res;

		next.prefix = NULL;
		oidcpy(&next.oid, null_oid());
		oidcpy(&next.suboid, null_oid());

		cp.dir = update_data->sm_path;

Tangentially related question (primarily for my own learning): in all of
these hunks, the string being xstrdup()-ed is update_data.sm_path, which
is only ever set in update_submodules():

		update_data->sm_path = ucd.sub->path;

where ucd.sub->path is a "const char *". So we never have to worry about
free()-ing update_data.sm_path, right? (Your patch doesn't attempt to
free() it, which sounds correct.)
Ævar Arnfjörð Bjarmason July 14, 2022, 2:43 p.m. UTC | #2 
On Wed, Jul 13 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Free the "dir" member of "struct child_process" that various functions
>> in submodule-helper.c allocate allocates with xstrdup().
>>
>> Since the "dir" argument is "const char *" let's keep a
>> "char *to_free" variable around for this rather than casting when we
>> call free().
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/submodule--helper.c | 41 +++++++++++++++++++++++++++----------
>>  1 file changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index a8e439e59b8..2099c5774b2 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2198,27 +2198,36 @@ static int is_tip_reachable(const char *path, struct object_id *oid)
>>  	struct child_process cp = CHILD_PROCESS_INIT;
>>  	struct strbuf rev = STRBUF_INIT;
>>  	char *hex = oid_to_hex(oid);
>> +	char *to_free;
>> +	int ret;
>>  
>>  	cp.git_cmd = 1;
>> -	cp.dir = xstrdup(path);
>> +	cp.dir = to_free = xstrdup(path);
>>  	cp.no_stderr = 1;
>>  	strvec_pushl(&cp.args, "rev-list", "-n", "1", hex, "--not", "--all", NULL);
>>  
>>  	prepare_submodule_repo_env(&cp.env);
>>  
>> -	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
>> -		return 0;
>> +	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len) {
>> +		ret = 0;
>> +		goto cleanup;
>> +	}
>>  
>> -	return 1;
>> +	ret = 1;
>> +cleanup:
>> +	free(to_free);
>> +	return ret;
>>  }
>>  
>>  static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)
>>  {
>>  	struct child_process cp = CHILD_PROCESS_INIT;
>> +	char *to_free;
>> +	int ret;
>>  
>>  	prepare_submodule_repo_env(&cp.env);
>>  	cp.git_cmd = 1;
>> -	cp.dir = xstrdup(module_path);
>> +	cp.dir = to_free = xstrdup(module_path);
>>  
>>  	strvec_push(&cp.args, "fetch");
>>  	if (quiet)
>> @@ -2232,7 +2241,9 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
>>  		free(remote);
>>  	}
>>  
>> -	return run_command(&cp);
>> +	ret = run_command(&cp);
>> +	free(to_free);
>> +	return ret;
>>  }
>>  
>>  static int run_update_command(struct update_data *ud, int subforce)
>> @@ -2240,6 +2251,8 @@ static int run_update_command(struct update_data *ud, int subforce)
>>  	struct child_process cp = CHILD_PROCESS_INIT;
>>  	char *oid = oid_to_hex(&ud->oid);
>>  	int must_die_on_failure = 0;
>> +	char *to_free;
>> +	int ret;
>>  
>>  	switch (ud->update_strategy.type) {
>>  	case SM_UPDATE_CHECKOUT:
>> @@ -2273,7 +2286,7 @@ static int run_update_command(struct update_data *ud, int subforce)
>>  	}
>>  	strvec_push(&cp.args, oid);
>>  
>> -	cp.dir = xstrdup(ud->sm_path);
>> +	cp.dir = to_free = xstrdup(ud->sm_path);
>>  	prepare_submodule_repo_env(&cp.env);
>>  	if (run_command(&cp)) {
>>  		switch (ud->update_strategy.type) {
>> @@ -2301,11 +2314,14 @@ static int run_update_command(struct update_data *ud, int subforce)
>>  			exit(128);
>>  
>>  		/* the command failed, but update must continue */
>> -		return 1;
>> +		ret = 1;
>> +		goto cleanup;
>>  	}
>>  
>> -	if (ud->quiet)
>> -		return 0;
>> +	if (ud->quiet) {
>> +		ret = 0;
>> +		goto cleanup;
>> +	}
>>  
>>  	switch (ud->update_strategy.type) {
>>  	case SM_UPDATE_CHECKOUT:
>> @@ -2329,7 +2345,10 @@ static int run_update_command(struct update_data *ud, int subforce)
>>  		    submodule_strategy_to_string(&ud->update_strategy));
>>  	}
>>  
>> -	return 0;
>> +	ret = 0;
>> +cleanup:
>> +	free(to_free);
>> +	return ret;
>>  }
>>  
>>  static int run_update_procedure(struct update_data *ud)
>
> I assume I'm missing something, but couldn't we achieve the same result
> by just removing the xstrdup() calls? i.e. we only leak .dir (which we
> don't clear because it's const), so couldn't we just assign to it
> without xstrdup() and not have to worry about free()-ing it?
>
> I didn't see a correctness reason for us to xstrdup(), and indeed, t7406
> passes with the change I just described (which I believe covers all of
> the sites here). In fact, we already have one site that does exactly
> this in the recursive part of update_submodule():
>
> 	if (update_data->recursive) {
> 		struct child_process cp = CHILD_PROCESS_INIT;
> 		struct update_data next = *update_data;
> 		int res;
>
> 		next.prefix = NULL;
> 		oidcpy(&next.oid, null_oid());
> 		oidcpy(&next.suboid, null_oid());
>
> 		cp.dir = update_data->sm_path;
>
> Tangentially related question (primarily for my own learning): in all of
> these hunks, the string being xstrdup()-ed is update_data.sm_path, which
> is only ever set in update_submodules():
>
> 		update_data->sm_path = ucd.sub->path;
>
> where ucd.sub->path is a "const char *". So we never have to worry about
> free()-ing update_data.sm_path, right? (Your patch doesn't attempt to
> free() it, which sounds correct.)

Yes, do'h! That's a much better fix, and only 3 lines of getting rid of
the xstrdup(). I'll re-roll with that.
diff mbox series

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a8e439e59b8..2099c5774b2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2198,27 +2198,36 @@  static int is_tip_reachable(const char *path, struct object_id *oid)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf rev = STRBUF_INIT;
 	char *hex = oid_to_hex(oid);
+	char *to_free;
+	int ret;
 
 	cp.git_cmd = 1;
-	cp.dir = xstrdup(path);
+	cp.dir = to_free = xstrdup(path);
 	cp.no_stderr = 1;
 	strvec_pushl(&cp.args, "rev-list", "-n", "1", hex, "--not", "--all", NULL);
 
 	prepare_submodule_repo_env(&cp.env);
 
-	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)
-		return 0;
+	if (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len) {
+		ret = 0;
+		goto cleanup;
+	}
 
-	return 1;
+	ret = 1;
+cleanup:
+	free(to_free);
+	return ret;
 }
 
 static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
+	char *to_free;
+	int ret;
 
 	prepare_submodule_repo_env(&cp.env);
 	cp.git_cmd = 1;
-	cp.dir = xstrdup(module_path);
+	cp.dir = to_free = xstrdup(module_path);
 
 	strvec_push(&cp.args, "fetch");
 	if (quiet)
@@ -2232,7 +2241,9 @@  static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
 		free(remote);
 	}
 
-	return run_command(&cp);
+	ret = run_command(&cp);
+	free(to_free);
+	return ret;
 }
 
 static int run_update_command(struct update_data *ud, int subforce)
@@ -2240,6 +2251,8 @@  static int run_update_command(struct update_data *ud, int subforce)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	char *oid = oid_to_hex(&ud->oid);
 	int must_die_on_failure = 0;
+	char *to_free;
+	int ret;
 
 	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
@@ -2273,7 +2286,7 @@  static int run_update_command(struct update_data *ud, int subforce)
 	}
 	strvec_push(&cp.args, oid);
 
-	cp.dir = xstrdup(ud->sm_path);
+	cp.dir = to_free = xstrdup(ud->sm_path);
 	prepare_submodule_repo_env(&cp.env);
 	if (run_command(&cp)) {
 		switch (ud->update_strategy.type) {
@@ -2301,11 +2314,14 @@  static int run_update_command(struct update_data *ud, int subforce)
 			exit(128);
 
 		/* the command failed, but update must continue */
-		return 1;
+		ret = 1;
+		goto cleanup;
 	}
 
-	if (ud->quiet)
-		return 0;
+	if (ud->quiet) {
+		ret = 0;
+		goto cleanup;
+	}
 
 	switch (ud->update_strategy.type) {
 	case SM_UPDATE_CHECKOUT:
@@ -2329,7 +2345,10 @@  static int run_update_command(struct update_data *ud, int subforce)
 		    submodule_strategy_to_string(&ud->update_strategy));
 	}
 
-	return 0;
+	ret = 0;
+cleanup:
+	free(to_free);
+	return ret;
 }
 
 static int run_update_procedure(struct update_data *ud)