On Wed, Jul 13 2022, Glen Choo wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
