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 <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 11, 2022 at 01:03 UTC 2a01bdedf87d7cbfc4411ff5059cfe406e1637db
11 files changed +27 -15
builtin/clone.c
+1 -1
@@ -72,7 +72,7 @@ static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
72 static int option_dissociate;
73 static int max_jobs = -1;
74 static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
75 -static struct list_objects_filter_options filter_options;
75 +static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
76 static int option_filter_submodules = -1; /* unspecified */
77 static int config_filter_submodules = -1; /* unspecified */
78 static struct string_list server_options = STRING_LIST_INIT_NODUP;
builtin/fetch-pack.c
+1
@@ -62,6 +62,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
62 packet_trace_identity("fetch-pack");
63
64 memset(&args, 0, sizeof(args));
65 + list_objects_filter_init(&args.filter_options);
66 args.uploadpack = "git-upload-pack";
67
68 for (i = 1; i < argc && *argv[i] == '-'; i++) {
builtin/fetch.c
+1 -1
@@ -80,7 +80,7 @@ static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
80 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
81 static int shown_url = 0;
82 static struct refspec refmap = REFSPEC_INIT_FETCH;
83 -static struct list_objects_filter_options filter_options;
83 +static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
84 static struct string_list server_options = STRING_LIST_INIT_DUP;
85 static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
86 static int fetch_write_commit_graph = -1;
builtin/submodule--helper.c
+4 -4
@@ -1754,7 +1754,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
1754 {
1755 int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
1756 struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
1757 - struct list_objects_filter_options filter_options;
1757 + struct list_objects_filter_options filter_options =
1758 + LIST_OBJECTS_FILTER_INIT;
1759
1760 struct option module_clone_options[] = {
1761 OPT_STRING(0, "prefix", &clone_data.prefix,
@@ -1796,7 +1797,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
1797 NULL
1798 };
1799
1799 - memset(&filter_options, 0, sizeof(filter_options));
1800 argc = parse_options(argc, argv, prefix, module_clone_options,
1801 git_submodule_helper_usage, 0);
1802
@@ -2581,7 +2581,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
2581 {
2582 struct pathspec pathspec;
2583 struct update_data opt = UPDATE_DATA_INIT;
2584 - struct list_objects_filter_options filter_options;
2584 + struct list_objects_filter_options filter_options =
2585 + LIST_OBJECTS_FILTER_INIT;
2586 int ret;
2587
2588 struct option module_update_options[] = {
@@ -2639,7 +2640,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
2640 update_clone_config_from_gitmodules(&opt.max_jobs);
2641 git_config(git_update_clone_config, &opt.max_jobs);
2642
2642 - memset(&filter_options, 0, sizeof(filter_options));
2643 argc = parse_options(argc, argv, prefix, module_update_options,
2644 git_submodule_helper_usage, 0);
2645
bundle.h
+1
@@ -18,6 +18,7 @@ struct bundle_header {
18 { \
19 .prerequisites = STRING_LIST_INIT_DUP, \
20 .references = STRING_LIST_INIT_DUP, \
21 + .filter = LIST_OBJECTS_FILTER_INIT, \
22 }
23 void bundle_header_init(struct bundle_header *header);
24 void bundle_header_release(struct bundle_header *header);
list-objects-filter-options.c
+11 -9
@@ -108,7 +108,7 @@ int gently_parse_list_objects_filter(
108
109 strbuf_addf(errbuf, _("invalid filter-spec '%s'"), arg);
110
111 - memset(filter_options, 0, sizeof(*filter_options));
111 + list_objects_filter_init(filter_options);
112 return 1;
113 }
114
@@ -223,8 +223,7 @@ static void transform_to_combine_type(
223 struct list_objects_filter_options *sub_array =
224 xcalloc(initial_sub_alloc, sizeof(*sub_array));
225 sub_array[0] = *filter_options;
226 - memset(filter_options, 0, sizeof(*filter_options));
227 - string_list_init_dup(&filter_options->filter_spec);
226 + list_objects_filter_init(filter_options);
227 filter_options->sub = sub_array;
228 filter_options->sub_alloc = initial_sub_alloc;
229 }
@@ -255,11 +254,8 @@ void parse_list_objects_filter(
254 struct strbuf errbuf = STRBUF_INIT;
255 int parse_error;
256
258 - if (!filter_options->filter_spec.strdup_strings) {
259 - if (filter_options->filter_spec.nr)
260 - BUG("unexpected non-allocated string in filter_spec");
261 - filter_options->filter_spec.strdup_strings = 1;
262 - }
257 + if (!filter_options->filter_spec.strdup_strings)
258 + BUG("filter_options not properly initialized");
259
260 if (!filter_options->choice) {
261 string_list_append(&filter_options->filter_spec, arg);
@@ -346,7 +342,7 @@ void list_objects_filter_release(
342 for (sub = 0; sub < filter_options->sub_nr; sub++)
343 list_objects_filter_release(&filter_options->sub[sub]);
344 free(filter_options->sub);
349 - memset(filter_options, 0, sizeof(*filter_options));
345 + list_objects_filter_init(filter_options);
346 }
347
348 void partial_clone_register(
@@ -429,3 +425,9 @@ void list_objects_filter_copy(
425 for (i = 0; i < src->sub_nr; i++)
426 list_objects_filter_copy(&dest->sub[i], &src->sub[i]);
427 }
428 +
429 +void list_objects_filter_init(struct list_objects_filter_options *filter_options)
430 +{
431 + struct list_objects_filter_options blank = LIST_OBJECTS_FILTER_INIT;
432 + memcpy(filter_options, &blank, sizeof(*filter_options));
433 +}
list-objects-filter-options.h
+3
@@ -69,6 +69,9 @@ struct list_objects_filter_options {
69 */
70 };
71
72 +#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP }
73 +void list_objects_filter_init(struct list_objects_filter_options *filter_options);
74 +
75 /*
76 * Parse value of the argument to the "filter" keyword.
77 * On the command line this looks like:
revision.c
+1
@@ -1900,6 +1900,7 @@ void repo_init_revisions(struct repository *r,
1900 }
1901
1902 init_display_notes(&revs->notes_opt);
1903 + list_objects_filter_init(&revs->filter);
1904 }
1905
1906 static void add_pending_commit_list(struct rev_info *revs,
transport-helper.c
+2
@@ -1286,6 +1286,8 @@ int transport_helper_init(struct transport *transport, const char *name)
1286 if (getenv("GIT_TRANSPORT_HELPER_DEBUG"))
1287 debug = 1;
1288
1289 + list_objects_filter_init(&data->transport_options.filter_options);
1290 +
1291 transport->data = data;
1292 transport->vtable = &vtable;
1293 transport->smart_options = &(data->transport_options);
transport.c
+1
@@ -1113,6 +1113,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
1113 * will be checked individually in git_connect.
1114 */
1115 struct git_transport_data *data = xcalloc(1, sizeof(*data));
1116 + list_objects_filter_init(&data->options.filter_options);
1117 ret->data = data;
1118 ret->vtable = &builtin_smart_vtable;
1119 ret->smart_options = &(data->options);
upload-pack.c
+1
@@ -141,6 +141,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
141 data->allow_filter_fallback = 1;
142 data->tree_filter_max_depth = ULONG_MAX;
143 packet_writer_init(&data->writer, 1);
144 + list_objects_filter_init(&data->filter_options);
145
146 data->keepalive = 5;
147 data->advertise_sid = 0;