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

> Æ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.)