Re: [PATCH v6 16/17] submodule--helper: free rest of "displaypath" in "struct update_data"

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Fix a leak in code added in c51f8f94e5b (submodule--helper: run update
> procedures from C, 2021-08-24), we clobber the "displaypath" member of
> the passed-in "struct update_data" both so that die() messages in this
> update_submodule() function itself can use it, and for the
> run_update_procedure() called within this function.
>
> To make managing that clobbering easier let's wrap the
> update_submodule() in a new update_submodule_outer() function, which
> will do the clobbering and free(to_free) dance for us.

The only feedback I had on v5 was on this patch
(https://lore.kernel.org/git/kl6l5yj7ubpt.fsf@chooglen-macbookpro.roam.corp.google.com)
where I said that update_submodule_outer() seemed like overkill and that
I'd test an alternative.

I tested this approach

  Assign and FREE_AND_NULL() update_data->displaypath [in
  update_submodules(), but outside of update_submodule()] since this
  is the only caller and it already does some prep work in this hunk.

by reverting this patch and applying this one

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3557665261..c932c857dd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2486,9 +2486,6 @@ static int update_submodule(struct update_data *update_data)
 	if (ret)
 		return ret;
 
-	update_data->displaypath = get_submodule_displaypath(
-		update_data->sm_path, update_data->prefix);
-
 	ret = determine_submodule_update_strategy(the_repository,
 						  update_data->just_cloned,
 						  update_data->sm_path,
@@ -2593,8 +2590,11 @@ static int update_submodules(struct update_data *update_data)
 		oidcpy(&update_data->oid, &ucd.oid);
 		update_data->just_cloned = ucd.just_cloned;
 		update_data->sm_path = ucd.sub->path;
+		update_data->displaypath = get_submodule_displaypath(
+			update_data->sm_path, update_data->prefix);
 
 		code = update_submodule(update_data);
+		FREE_AND_NULL(update_data->displaypath);
 		if (!code)
 			continue;
 		ret = code;
----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

Then I tested the update_submodule_outer() version and the
FREE_AND_NULL() version by marking t7406-submodule-update.sh as
TEST_PASSES_SANITIZE_LEAK=true and diffing the test output. I didn't see
any meaningful difference in the test output, which should mean that
both versions fix all of the leaks in "git submodule update" that our
test suite can catch.

For good measure, I also tested a version of my code without the
FREE_AND_NULL() call (albeit on v5, not v6), and I spotted the expected
memory leak:

  Direct leak of 10 byte(s) in 1 object(s) allocated from:
      #0 0x000000000000 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54
      #1 0x000000000000 in strdup (/lib/x86_64-linux-gnu/libc.so.6+0x000000000000)
      #2 0x000000000000 in xstrdup wrapper.c:39
      #3 0x000000000000 in get_submodule_displaypath builtin/submodule--helper.c:119
      #4 0x000000000000 in update_submodules builtin/submodule--helper.c:2585
      #5 0x000000000000 in module_update builtin/submodule--helper.c:2716
      #6 0x000000000000 in run_builtin git.c:466
      #7 0x000000000000 in handle_builtin git.c:720
      #8 0x000000000000 in run_argv git.c:787
      #9 0x000000000000 in cmd_main git.c:920
      #10 0x000000000000 in main common-main.c:56
      #11 0x000000000000 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x000000000000)
      #12 0x000000000000 in _start (git+0x000000000000)

The CI runs are at: 
- FREE_AND_NULL(): https://github.com/chooglen/git/actions/runs/2920894120
- update_submodule_outer(): https://github.com/chooglen/git/actions/runs/2915661611
- control study: https://github.com/chooglen/git/actions/runs/2915671776

If FREE_AND_NULL() really plugs all of the leaks, I would strongly
prefer using that (possibly in a "goto cleanup"), over introducing
update_submodule_outer().