| Message ID | patch-05.20-49e6714939d-20221228T175512Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: various simple leak fixes | expand |
Am 28.12.22 um 19:00 schrieb Ævar Arnfjörð Bjarmason: > Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further > pointers when finished, 2021-03-14) to use a "to_free" pattern > instead. In this case the "repo" can be either this absolute_pathdup() > value, or in the "else if" branch seen in the context the the > "argv[0]" argument to "main()". > > We can only free() the value in the former case, hence the "to_free" > pattern. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/clone.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index f518bb2dc1f..48156a4f2c2 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -892,6 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > int is_bundle = 0, is_local; > int reject_shallow = 0; > const char *repo_name, *repo, *work_tree, *git_dir; > + char *repo_to_free = NULL; > char *path = NULL, *dir, *display_repo = NULL; > int dest_exists, real_dest_exists = 0; > const struct ref *refs, *remote_head; > @@ -949,7 +950,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > path = get_repo_path(repo_name, &is_bundle); > if (path) { > FREE_AND_NULL(path); > - repo = absolute_pathdup(repo_name); > + repo = repo_to_free = absolute_pathdup(repo_name); > } else if (strchr(repo_name, ':')) { > repo = repo_name; Alternatively you could do "repo = xstrdup(repo_name);" here to simplify memory ownership of this string, at the cost of a small allocation. But the approach taken by this patch is fine as well. > display_repo = transport_anonymize_url(repo); > @@ -1392,7 +1393,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > free(unborn_head); > free(dir); > free(path); > - UNLEAK(repo); > + free(repo_to_free); > junk_mode = JUNK_LEAVE_ALL; > > transport_ls_refs_options_release(&transport_ls_refs_options);
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further > pointers when finished, 2021-03-14) to use a "to_free" pattern > instead. In this case the "repo" can be either this absolute_pathdup() > value, or in the "else if" branch seen in the context the the > "argv[0]" argument to "main()". > > We can only free() the value in the former case, hence the "to_free" > pattern. Many other patches in the series, especially the ones that touch the library-ish parts that can be called unbounded number of times, do make sense, but this patch helps nobody. Not even the leak checker, who should already be happy with the existing UNLEAK() marking. The only thing it does is to free() a piece of memory obtained from a one-time allocation that will be discarded upon program exit anyway. If this were a freestanding patch done outside any series, I would probably have not bothered to queue it. I'll be queuing as part of the larger whole, but I am not all that impressed by this step and the thinking that led to its inclusion in the series. Thanks. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/clone.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index f518bb2dc1f..48156a4f2c2 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -892,6 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > int is_bundle = 0, is_local; > int reject_shallow = 0; > const char *repo_name, *repo, *work_tree, *git_dir; > + char *repo_to_free = NULL; > char *path = NULL, *dir, *display_repo = NULL; > int dest_exists, real_dest_exists = 0; > const struct ref *refs, *remote_head; > @@ -949,7 +950,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > path = get_repo_path(repo_name, &is_bundle); > if (path) { > FREE_AND_NULL(path); > - repo = absolute_pathdup(repo_name); > + repo = repo_to_free = absolute_pathdup(repo_name); > } else if (strchr(repo_name, ':')) { > repo = repo_name; > display_repo = transport_anonymize_url(repo); > @@ -1392,7 +1393,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > free(unborn_head); > free(dir); > free(path); > - UNLEAK(repo); > + free(repo_to_free); > junk_mode = JUNK_LEAVE_ALL; > > transport_ls_refs_options_release(&transport_ls_refs_options);
On Thu, Dec 29 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further >> pointers when finished, 2021-03-14) to use a "to_free" pattern >> instead. In this case the "repo" can be either this absolute_pathdup() >> value, or in the "else if" branch seen in the context the the >> "argv[0]" argument to "main()". >> >> We can only free() the value in the former case, hence the "to_free" >> pattern. > > Many other patches in the series, especially the ones that touch the > library-ish parts that can be called unbounded number of times, do > make sense, but this patch helps nobody. Not even the leak checker, > who should already be happy with the existing UNLEAK() marking. The > only thing it does is to free() a piece of memory obtained from a > one-time allocation that will be discarded upon program exit anyway. The changes in this series that deal with UNLEAK() take it as a given that it's a good thing to replace these trivial cases with a free(). But the rationale is in ac95f5d36ad (built-ins: use free() not UNLEAK() if trivial, rm dead code, 2022-11-08) in the earlier topic, this is the follow-up. We only have 15 UNLEAK() invocations in-tree left, with this series (which made no special effort to get the rest, but just the remaining very low hanging fruit) it's 13. If our built-ins were using this pattern consistently I think it would be worth keeping & using UNLEAK() like this, but they aren't. Even the "display_repo" variable seen in the context of this patch is free()'d, see 46da295a770 (clone/fetch: anonymize URLs in the reflog, 2020-06-04), along with the rest in this function and others (with the exception of the remaining UNLEAK(...)), e.g. "remote_name" etc. So I think keeping it just makes the code more confusing, one wonders why this particular variable is being UNLEAK()'d, instead of just free()'d like the rest. As the comment on the SUPPRESS_ANNOTATED_LEAKS macro notes we only get the benefit from it if you compile with SUPPRESS_ANNOTATED_LEAKS, so e.g. if you use SANITIZE=leak it'll kick in, but not on a normal build and e.g.: valgrind --leak-check=full ./git commit-graph verify Which will with UNLEAK() report the leak fixed in 04/20 here (I'm just using it in this example since easier to reproduce), but with free() we won't report it. I agree it has a marginal benefit, e.g. it won't contribute to the linux-leaks job, as it uses SUPPRESS_ANNOTATED_LEAKS, but the above is why I think it's worth changing the remaining low-hanging UNLEAK() fruit to free(). We do have cases where it's much tricker to replace those UNLEAK() with free(). I think it's much better if we narrow its use to those cases at this point.
diff --git a/builtin/clone.c b/builtin/clone.c index f518bb2dc1f..48156a4f2c2 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -892,6 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int is_bundle = 0, is_local; int reject_shallow = 0; const char *repo_name, *repo, *work_tree, *git_dir; + char *repo_to_free = NULL; char *path = NULL, *dir, *display_repo = NULL; int dest_exists, real_dest_exists = 0; const struct ref *refs, *remote_head; @@ -949,7 +950,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) path = get_repo_path(repo_name, &is_bundle); if (path) { FREE_AND_NULL(path); - repo = absolute_pathdup(repo_name); + repo = repo_to_free = absolute_pathdup(repo_name); } else if (strchr(repo_name, ':')) { repo = repo_name; display_repo = transport_anonymize_url(repo); @@ -1392,7 +1393,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) free(unborn_head); free(dir); free(path); - UNLEAK(repo); + free(repo_to_free); junk_mode = JUNK_LEAVE_ALL; transport_ls_refs_options_release(&transport_ls_refs_options);
Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further pointers when finished, 2021-03-14) to use a "to_free" pattern instead. In this case the "repo" can be either this absolute_pathdup() value, or in the "else if" branch seen in the context the the "argv[0]" argument to "main()". We can only free() the value in the former case, hence the "to_free" pattern. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/clone.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)