| Message ID | 20240909232118.GI921834@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | db629c61f0be3665a36750fe2353b9ee958b0376 |
| Headers | show |
| Series | ref-filter %(trailer) fixes | expand |
On Mon, Sep 09, 2024 at 07:21:18PM -0400, Jeff King wrote: > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index e8db612f95..b3163629c5 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -5,6 +5,7 @@ > > test_description='for-each-ref test' > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > GNUPGHOME_NOT_USED=$GNUPGHOME > . "$TEST_DIRECTORY"/lib-gpg.sh Nice! There's also t6302, which has been failing due to all the memory leaks in our atom handling, as well. After your series there's a single memory leak left to make it pass. So we may want to add below patch on top as a low-hanging fruit. Patrick -- >8 -- diff --git a/ref-filter.c b/ref-filter.c index ce1bcfad857..b06e18a569a 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1001,6 +1001,7 @@ struct ref_formatting_stack { struct ref_formatting_stack *prev; struct strbuf output; void (*at_end)(struct ref_formatting_stack **stack); + void (*at_end_data_free)(void *data); void *at_end_data; }; @@ -1169,6 +1170,8 @@ static void pop_stack_element(struct ref_formatting_stack **stack) if (prev) strbuf_addbuf(&prev->output, ¤t->output); strbuf_release(¤t->output); + if (current->at_end_data_free) + current->at_end_data_free(current->at_end_data); free(current); *stack = prev; } @@ -1228,15 +1231,13 @@ static void if_then_else_handler(struct ref_formatting_stack **stack) } *stack = cur; - free(if_then_else); } static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state, struct strbuf *err UNUSED) { struct ref_formatting_stack *new_stack; - struct if_then_else *if_then_else = xcalloc(1, - sizeof(struct if_then_else)); + struct if_then_else *if_then_else = xcalloc(1, sizeof(*if_then_else)); if_then_else->str = atomv->atom->u.if_then_else.str; if_then_else->cmp_status = atomv->atom->u.if_then_else.cmp_status; @@ -1245,6 +1246,7 @@ static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state new_stack = state->stack; new_stack->at_end = if_then_else_handler; new_stack->at_end_data = if_then_else; + new_stack->at_end_data_free = free; return 0; } diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 163c378cfd1..7f44d3c3f22 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -2,6 +2,7 @@ test_description='test for-each-refs usage of ref-filter APIs' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh
On Tue, Sep 10, 2024 at 08:09:16AM +0200, Patrick Steinhardt wrote: > On Mon, Sep 09, 2024 at 07:21:18PM -0400, Jeff King wrote: > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > > index e8db612f95..b3163629c5 100755 > > --- a/t/t6300-for-each-ref.sh > > +++ b/t/t6300-for-each-ref.sh > > @@ -5,6 +5,7 @@ > > > > test_description='for-each-ref test' > > > > +TEST_PASSES_SANITIZE_LEAK=true > > . ./test-lib.sh > > GNUPGHOME_NOT_USED=$GNUPGHOME > > . "$TEST_DIRECTORY"/lib-gpg.sh > > Nice! There's also t6302, which has been failing due to all the memory > leaks in our atom handling, as well. After your series there's a single > memory leak left to make it pass. So we may want to add below patch on > top as a low-hanging fruit. I was afraid to go looking for other almost-there scripts, knowing what a rabbit hole it can turn into (which I know you are also familiar with). > -- >8 -- > diff --git a/ref-filter.c b/ref-filter.c > index ce1bcfad857..b06e18a569a 100644 > --- a/ref-filter.c > +++ b/ref-filter.c This looks plausibly correct to me, but I'm not at all familiar with the conditional placeholders. I think it would make more sense for you to wrap it up with a commit message. -Peff
diff --git a/builtin/branch.c b/builtin/branch.c index 3f870741bf..c98601c6fe 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -878,6 +878,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) string_list_clear(&output, 0); ref_sorting_release(sorting); ref_filter_clear(&filter); + ref_format_clear(&format); return 0; } else if (edit_description) { const char *branch_name; diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 5517a4a1c0..c72fa05bcb 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -104,6 +104,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) filter_and_format_refs(&filter, flags, sorting, &format); ref_filter_clear(&filter); + ref_format_clear(&format); ref_sorting_release(sorting); strvec_clear(&vec); return 0; diff --git a/builtin/tag.c b/builtin/tag.c index a1fb218512..607e48e311 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -702,6 +702,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) cleanup: ref_sorting_release(sorting); ref_filter_clear(&filter); + ref_format_clear(&format); strbuf_release(&buf); strbuf_release(&ref); strbuf_release(&reflog_msg); diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index c731e2f87b..77becf7e75 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -65,5 +65,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) if (format.format) pretty_print_ref(name, &oid, &format); } + ref_format_clear(&format); return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 0f51095bbd..ce1bcfad85 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3621,3 +3621,16 @@ void ref_filter_clear(struct ref_filter *filter) free_commit_list(filter->unreachable_from); ref_filter_init(filter); } + +void ref_format_init(struct ref_format *format) +{ + struct ref_format blank = REF_FORMAT_INIT; + memcpy(format, &blank, sizeof(blank)); +} + +void ref_format_clear(struct ref_format *format) +{ + string_list_clear(&format->bases, 0); + string_list_clear(&format->is_base_tips, 0); + ref_format_init(format); +} diff --git a/ref-filter.h b/ref-filter.h index e794b8a676..754038ab07 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -221,4 +221,7 @@ void filter_is_base(struct repository *r, void ref_filter_init(struct ref_filter *filter); void ref_filter_clear(struct ref_filter *filter); +void ref_format_init(struct ref_format *format); +void ref_format_clear(struct ref_format *format); + #endif /* REF_FILTER_H */ diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index e8db612f95..b3163629c5 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -5,6 +5,7 @@ test_description='for-each-ref test' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GNUPGHOME_NOT_USED=$GNUPGHOME . "$TEST_DIRECTORY"/lib-gpg.sh
After using the ref-filter API, callers should use ref_filter_clear() to free any used memory. However, there's not a matching function to clear the ref_format struct. Traditionally this did not need to be cleaned up, as it was just a way for the caller to store and pass format options as a single unit. Even though the parsing step of some placeholders may allocate data, that's usually inside their "used_atom" structs, which are part of the ref_filter itself. But a few placeholders keep data outside of there. The %(ahead-behind) and %(is-base) parsers both keep a master list of bases, because they perform a single filtering pass outside of the use of any particular atom. And since the format parser does not have access to the ref_filter struct, they store their cross-atom data in the ref_format struct itself. And thus when they are finished, the ref_format also needs to be cleaned up. So let's add a function to do so, and call it from all of the users of the ref-filter API. The %(is-base) case is found by running LSan on t6300. After this patch, the script can now be marked leak-free. Signed-off-by: Jeff King <peff@peff.net> --- builtin/branch.c | 1 + builtin/for-each-ref.c | 1 + builtin/tag.c | 1 + builtin/verify-tag.c | 1 + ref-filter.c | 13 +++++++++++++ ref-filter.h | 3 +++ t/t6300-for-each-ref.sh | 1 + 7 files changed, 21 insertions(+)