From 2a01bdedf87d7cbfc4411ff5059cfe406e1637db Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 11 Sep 2022 01:03:07 -0400 Subject: list-objects-filter: add and use initializers In 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec strings, 2022-09-08), we noted that the filter_spec string_list was inconsistent in how it handled memory ownership of strings stored in the list. The fix there was a bit of a band-aid to set the "strdup_strings" variable right before adding anything. That works OK, and it lets the users of the API continue to zero-initialize the struct. But it makes the code a bit hard to follow and accident-prone, as any other spots appending the filter_spec need to think about whether to set the strdup_strings value, too (there's one such spot in partial_clone_get_default_filter_spec(), which is probably a possible memory leak). So let's do that full cleanup now. We'll introduce a LIST_OBJECTS_FILTER_INIT macro and matching function, and use them as appropriate (though it is for the "_options" struct, this matches the corresponding list_objects_filter_release() function). This is harder than it seems! Many other structs, like git_transport_data, embed the filter struct. So they need to initialize it themselves even if the rest of the enclosing struct is OK with zero-initialization. I found all of the relevant spots by grepping manually for declarations of list_objects_filter_options. And then doing so recursively for structs which embed it, and ones which embed those, and so on. I'm pretty sure I got everything, but there's no change that would alert the compiler if any topics in flight added new declarations. To catch this case, we now double-check in the parsing function that things were initialized as expected and BUG() if appropriate. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- list-objects-filter-options.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'list-objects-filter-options.h') diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h index ffc02d77e7..2720f7dba8 100644 --- a/list-objects-filter-options.h +++ b/list-objects-filter-options.h @@ -69,6 +69,9 @@ struct list_objects_filter_options { */ }; +#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP } +void list_objects_filter_init(struct list_objects_filter_options *filter_options); + /* * Parse value of the argument to the "filter" keyword. * On the command line this looks like: -- cgit v1.2.3 From c54980ab83661e8e290003fbd5ab44b12e4e77b1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 11 Sep 2022 01:03:31 -0400 Subject: list-objects-filter: convert filter_spec to a strbuf Originally, the filter_spec field was just a string pointer. In cf9ceb5a12 (list-objects-filter-options: make filter_spec a string_list, 2019-06-27) it became a string_list, but that commit notes: A strbuf would seem to be a more natural choice for this object, but it unfortunately requires initialization besides just zero'ing out the memory. This results in all container structs, and all containers of those structs, etc., to also require initialization. Initializing them all would be more cumbersome that simply using a string_list, which behaves properly when its contents are zero'd. Now that we've changed the struct to require non-zero initialization anyway (ironically, because string_list also needed non-zero initialization to avoid leaks), we can now convert to that more natural type. This makes the list_objects_filter_spec() function much less awkward, as it had to collapse the string_list to a single-entry list on the fly. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- list-objects-filter-options.c | 51 +++++++++++++++---------------------------- list-objects-filter-options.h | 4 ++-- 2 files changed, 20 insertions(+), 35 deletions(-) (limited to 'list-objects-filter-options.h') diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 56a1933a50..d46ce4acc4 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -202,10 +202,10 @@ static int allow_unencoded(char ch) static void filter_spec_append_urlencode( struct list_objects_filter_options *filter, const char *raw) { - struct strbuf buf = STRBUF_INIT; - strbuf_addstr_urlencode(&buf, raw, allow_unencoded); - trace_printf("Add to combine filter-spec: %s\n", buf.buf); - string_list_append_nodup(&filter->filter_spec, strbuf_detach(&buf, NULL)); + size_t orig_len = filter->filter_spec.len; + strbuf_addstr_urlencode(&filter->filter_spec, raw, allow_unencoded); + trace_printf("Add to combine filter-spec: %s\n", + filter->filter_spec.buf + orig_len); } /* @@ -229,7 +229,7 @@ static void transform_to_combine_type( } filter_options->sub_nr = 1; filter_options->choice = LOFC_COMBINE; - string_list_append(&filter_options->filter_spec, "combine:"); + strbuf_addstr(&filter_options->filter_spec, "combine:"); filter_spec_append_urlencode( filter_options, list_objects_filter_spec(&filter_options->sub[0])); @@ -237,7 +237,7 @@ static void transform_to_combine_type( * We don't need the filter_spec strings for subfilter specs, only the * top level. */ - string_list_clear(&filter_options->sub[0].filter_spec, /*free_util=*/0); + strbuf_release(&filter_options->sub[0].filter_spec); } void list_objects_filter_die_if_populated( @@ -254,11 +254,11 @@ void parse_list_objects_filter( struct strbuf errbuf = STRBUF_INIT; int parse_error; - if (!filter_options->filter_spec.strdup_strings) + if (!filter_options->filter_spec.buf) BUG("filter_options not properly initialized"); if (!filter_options->choice) { - string_list_append(&filter_options->filter_spec, arg); + strbuf_addstr(&filter_options->filter_spec, arg); parse_error = gently_parse_list_objects_filter( filter_options, arg, &errbuf); @@ -269,7 +269,7 @@ void parse_list_objects_filter( */ transform_to_combine_type(filter_options); - string_list_append(&filter_options->filter_spec, "+"); + strbuf_addch(&filter_options->filter_spec, '+'); filter_spec_append_urlencode(filter_options, arg); ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, filter_options->sub_alloc); @@ -300,31 +300,18 @@ int opt_parse_list_objects_filter(const struct option *opt, const char *list_objects_filter_spec(struct list_objects_filter_options *filter) { - if (!filter->filter_spec.nr) + if (!filter->filter_spec.len) BUG("no filter_spec available for this filter"); - if (filter->filter_spec.nr != 1) { - struct strbuf concatted = STRBUF_INIT; - strbuf_add_separated_string_list( - &concatted, "", &filter->filter_spec); - string_list_clear(&filter->filter_spec, /*free_util=*/0); - string_list_append_nodup( - &filter->filter_spec, strbuf_detach(&concatted, NULL)); - } - - return filter->filter_spec.items[0].string; + return filter->filter_spec.buf; } const char *expand_list_objects_filter_spec( struct list_objects_filter_options *filter) { if (filter->choice == LOFC_BLOB_LIMIT) { - struct strbuf expanded_spec = STRBUF_INIT; - strbuf_addf(&expanded_spec, "blob:limit=%lu", + strbuf_release(&filter->filter_spec); + strbuf_addf(&filter->filter_spec, "blob:limit=%lu", filter->blob_limit_value); - string_list_clear(&filter->filter_spec, /*free_util=*/0); - string_list_append_nodup( - &filter->filter_spec, - strbuf_detach(&expanded_spec, NULL)); } return list_objects_filter_spec(filter); @@ -337,7 +324,7 @@ void list_objects_filter_release( if (!filter_options) return; - string_list_clear(&filter_options->filter_spec, /*free_util=*/0); + strbuf_release(&filter_options->filter_spec); free(filter_options->sparse_oid_name); for (sub = 0; sub < filter_options->sub_nr; sub++) list_objects_filter_release(&filter_options->sub[sub]); @@ -398,8 +385,8 @@ void partial_clone_get_default_filter_spec( if (!promisor || !promisor->partial_clone_filter) return; - string_list_append(&filter_options->filter_spec, - promisor->partial_clone_filter); + strbuf_addstr(&filter_options->filter_spec, + promisor->partial_clone_filter); gently_parse_list_objects_filter(filter_options, promisor->partial_clone_filter, &errbuf); @@ -411,14 +398,12 @@ void list_objects_filter_copy( const struct list_objects_filter_options *src) { int i; - struct string_list_item *item; /* Copy everything. We will overwrite the pointers shortly. */ memcpy(dest, src, sizeof(struct list_objects_filter_options)); - string_list_init_dup(&dest->filter_spec); - for_each_string_list_item(item, &src->filter_spec) - string_list_append(&dest->filter_spec, item->string); + strbuf_init(&dest->filter_spec, 0); + strbuf_addbuf(&dest->filter_spec, &src->filter_spec); dest->sparse_oid_name = xstrdup_or_null(src->sparse_oid_name); ALLOC_ARRAY(dest->sub, dest->sub_alloc); diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h index 2720f7dba8..7eeadab2dd 100644 --- a/list-objects-filter-options.h +++ b/list-objects-filter-options.h @@ -35,7 +35,7 @@ struct list_objects_filter_options { * To get the raw filter spec given by the user, use the result of * list_objects_filter_spec(). */ - struct string_list filter_spec; + struct strbuf filter_spec; /* * 'choice' is determined by parsing the filter-spec. This indicates @@ -69,7 +69,7 @@ struct list_objects_filter_options { */ }; -#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP } +#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRBUF_INIT } void list_objects_filter_init(struct list_objects_filter_options *filter_options); /* -- cgit v1.2.3