[PATCH 0/4] list-objects-filter cleanups

[PATCH 0/4] list-objects-filter cleanups

[Jeff King]

In the recent jk/plug-list-object-filter-leaks topic[1], I stopped short
of fixing all of the callers to actually initialize the filter struct
beyond zero-ing it.

This series does the cleanup that I was afraid to do there. :)

I think the end result is less confusing and error-prone. And as you can
see in patch 4, it matches how the code originally hoped to be written,
but the author was also afraid of the zero-initialization thing.

It is kind of churny, and carries some risk of regression (if I missed a
spot). IMHO it's worth it, but even if we don't take it, we should pick
up the first two patches, which are small bug-lets that the conversion
turned up.

These patches should be applied on top of jk/plug-list-object-filter-leaks,
which is currently in next.

  [1/4]: list-objects-filter: don't memset after releasing filter struct
  [2/4]: list-objects-filter: handle null default filter spec
  [3/4]: list-objects-filter: add and use initializers
  [4/4]: list-objects-filter: convert filter_spec to a strbuf

 builtin/clone.c               |  2 +-
 builtin/fetch-pack.c          |  1 +
 builtin/fetch.c               |  2 +-
 builtin/submodule--helper.c   |  8 ++--
 bundle.h                      |  1 +
 list-objects-filter-options.c | 75 ++++++++++++++---------------------
 list-objects-filter-options.h |  5 ++-
 revision.c                    |  1 +
 transport-helper.c            |  2 +
 transport.c                   |  1 +
 upload-pack.c                 |  1 +
 11 files changed, 47 insertions(+), 52 deletions(-)

-Peff

[1] https://lore.kernel.org/git/Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net/

[PATCH 1/4] list-objects-filter: don't memset after releasing filter struct

[Jeff King]

If we see an error while parsing a "combine" filter, we call
list_objects_filter_release() to free any allocated memory,
and then use memset() to return the struct to a known state. But the
release function already does that reinitializing. Doing it again is
pointless.

Signed-off-by: Jeff King <peff@peff.net>
---
 list-objects-filter-options.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 6cc4eb8e1c..ea989db260 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -187,10 +187,8 @@ static int parse_combine_filter(
 
 cleanup:
 	strbuf_list_free(subspecs);
-	if (result) {
+	if (result)
 		list_objects_filter_release(filter_options);
-		memset(filter_options, 0, sizeof(*filter_options));
-	}
 	return result;
 }
 
-- 
2.37.3.1242.g835d375f85

[PATCH 2/4] list-objects-filter: handle null default filter spec

[Jeff King]

When we have a remote.*.promisor config variable, we know that we're in
a partial clone. Usually there's a matching remote.*.partialclonefilter
option, which tells us which filter to use with the remote. If that
option is missing, we skip setting up the filter at all. But something
funny happens: we stick a NULL entry into the string_list storing the
text filter spec.

This is a weird state, and could possibly segfault if anybody called
called list_objects_filter_spec(), etc. In practice, nobody does,
because filter->choice will still be LOFC_DISABLED, so code generally
realizes there's no filter to use. And the string_list itself is OK,
because it starts in non-dup mode until we actually parse a filter spec.
So it blindly stores the NULL without even looking at it.

But it's probably worth avoiding this confused state. It's an accident
waiting to happen, and it will be a problem if we replace the lazy
initialization from 7e2619d8ff (list_objects_filter_options: plug leak
of filter_spec strings, 2022-09-08) with a real initialization function.

The history is a little interesting here, as the bug was introduced
during the merge resolution in 627b826834 (Merge branch
'md/list-objects-filter-combo', 2019-09-18).

The original logic comes from cac1137dc4 (list-objects: check if filter
is NULL before using, 2018-06-11), where we had a single string via
core.partialCloneFilter, and a simple NULL check was sufficient. And it
even added a test in t0410 that covers this situation.

Later, that was expanded to allow per-remote filters in fa3d1b63e8
(promisor-remote: parse remote.*.partialclonefilter, 2019-06-25). After
that commit, we get a promisor struct with a partial_clone_filter
string, which could be NULL. The commit checks only that the struct
pointer is non-NULL, which is enough. It may pass NULL to
gently_parse_list_objects_filter(), but that function is smart enough to
consider it a noop.

But in parallel, cf9ceb5a12 (list-objects-filter-options: make
filter_spec a string_list, 2019-06-27) added a new line of code: before
we call gently_parse_list_objets_filter(), we append the filter spec to
the string_list. By itself that was OK, since we'd have returned early
if the string was NULL.

When the two were merged in 627b826834, the result is that we return
early only if the struct is NULL, but not the string. And we append to
the string_list, meaning we may append NULL.

The solution is to return early if either is NULL, as it would mean we
don't have a configured filter.

Signed-off-by: Jeff King <peff@peff.net>
---
Sorry, I know it's a long message for a one-line fix! I found the
history of it so interesting, though, because it's rare that I see
bugs introduced during merges.

 list-objects-filter-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index ea989db260..18c51001dc 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -399,7 +399,7 @@ void partial_clone_get_default_filter_spec(
 	/*
 	 * Parse default value, but silently ignore it if it is invalid.
 	 */
-	if (!promisor)
+	if (!promisor || !promisor->partial_clone_filter)
 		return;
 
 	string_list_append(&filter_options->filter_spec,
-- 
2.37.3.1242.g835d375f85

[PATCH 3/4] list-objects-filter: add and use initializers

[Jeff King]

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>
---
 builtin/clone.c               |  2 +-
 builtin/fetch-pack.c          |  1 +
 builtin/fetch.c               |  2 +-
 builtin/submodule--helper.c   |  8 ++++----
 bundle.h                      |  1 +
 list-objects-filter-options.c | 20 +++++++++++---------
 list-objects-filter-options.h |  3 +++
 revision.c                    |  1 +
 transport-helper.c            |  2 ++
 transport.c                   |  1 +
 upload-pack.c                 |  1 +
 11 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index e21d42dfee..d269d6fec6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -73,7 +73,7 @@ static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
-static struct list_objects_filter_options filter_options;
+static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static int option_filter_submodules = -1;    /* unspecified */
 static int config_filter_submodules = -1;    /* unspecified */
 static struct string_list server_options = STRING_LIST_INIT_NODUP;
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f045bbbe94..afe679368d 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -62,6 +62,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	packet_trace_identity("fetch-pack");
 
 	memset(&args, 0, sizeof(args));
+	list_objects_filter_init(&args.filter_options);
 	args.uploadpack = "git-upload-pack";
 
 	for (i = 1; i < argc && *argv[i] == '-'; i++) {
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 368a0f5329..d2c0c65de4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -80,7 +80,7 @@ static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
 static int shown_url = 0;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
-static struct list_objects_filter_options filter_options;
+static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static struct string_list server_options = STRING_LIST_INIT_DUP;
 static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 static int fetch_write_commit_graph = -1;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b63f420ece..4b958e3cf8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1747,7 +1747,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 {
 	int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
 	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
-	struct list_objects_filter_options filter_options;
+	struct list_objects_filter_options filter_options =
+		LIST_OBJECTS_FILTER_INIT;
 
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &clone_data.prefix,
@@ -1789,7 +1790,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 		NULL
 	};
 
-	memset(&filter_options, 0, sizeof(filter_options));
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
@@ -2566,7 +2566,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
 {
 	struct pathspec pathspec;
 	struct update_data opt = UPDATE_DATA_INIT;
-	struct list_objects_filter_options filter_options;
+	struct list_objects_filter_options filter_options =
+		LIST_OBJECTS_FILTER_INIT;
 	int ret;
 
 	struct option module_update_options[] = {
@@ -2626,7 +2627,6 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	update_clone_config_from_gitmodules(&opt.max_jobs);
 	git_config(git_update_clone_config, &opt.max_jobs);
 
-	memset(&filter_options, 0, sizeof(filter_options));
 	argc = parse_options(argc, argv, prefix, module_update_options,
 			     git_submodule_helper_usage, 0);
 
diff --git a/bundle.h b/bundle.h
index 0c052f5496..68ff39a0a7 100644
--- a/bundle.h
+++ b/bundle.h
@@ -18,6 +18,7 @@ struct bundle_header {
 { \
 	.prerequisites = STRING_LIST_INIT_DUP, \
 	.references = STRING_LIST_INIT_DUP, \
+	.filter = LIST_OBJECTS_FILTER_INIT, \
 }
 void bundle_header_init(struct bundle_header *header);
 void bundle_header_release(struct bundle_header *header);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 18c51001dc..56a1933a50 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -108,7 +108,7 @@ int gently_parse_list_objects_filter(
 
 	strbuf_addf(errbuf, _("invalid filter-spec '%s'"), arg);
 
-	memset(filter_options, 0, sizeof(*filter_options));
+	list_objects_filter_init(filter_options);
 	return 1;
 }
 
@@ -223,8 +223,7 @@ static void transform_to_combine_type(
 		struct list_objects_filter_options *sub_array =
 			xcalloc(initial_sub_alloc, sizeof(*sub_array));
 		sub_array[0] = *filter_options;
-		memset(filter_options, 0, sizeof(*filter_options));
-		string_list_init_dup(&filter_options->filter_spec);
+		list_objects_filter_init(filter_options);
 		filter_options->sub = sub_array;
 		filter_options->sub_alloc = initial_sub_alloc;
 	}
@@ -255,11 +254,8 @@ 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.nr)
-			BUG("unexpected non-allocated string in filter_spec");
-		filter_options->filter_spec.strdup_strings = 1;
-	}
+	if (!filter_options->filter_spec.strdup_strings)
+		BUG("filter_options not properly initialized");
 
 	if (!filter_options->choice) {
 		string_list_append(&filter_options->filter_spec, arg);
@@ -346,7 +342,7 @@ void list_objects_filter_release(
 	for (sub = 0; sub < filter_options->sub_nr; sub++)
 		list_objects_filter_release(&filter_options->sub[sub]);
 	free(filter_options->sub);
-	memset(filter_options, 0, sizeof(*filter_options));
+	list_objects_filter_init(filter_options);
 }
 
 void partial_clone_register(
@@ -429,3 +425,9 @@ void list_objects_filter_copy(
 	for (i = 0; i < src->sub_nr; i++)
 		list_objects_filter_copy(&dest->sub[i], &src->sub[i]);
 }
+
+void list_objects_filter_init(struct list_objects_filter_options *filter_options)
+{
+	struct list_objects_filter_options blank = LIST_OBJECTS_FILTER_INIT;
+	memcpy(filter_options, &blank, sizeof(*filter_options));
+}
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:
diff --git a/revision.c b/revision.c
index a04ebd6139..7fc0e16bdf 100644
--- a/revision.c
+++ b/revision.c
@@ -1903,6 +1903,7 @@ void repo_init_revisions(struct repository *r,
 	}
 
 	init_display_notes(&revs->notes_opt);
+	list_objects_filter_init(&revs->filter);
 }
 
 static void add_pending_commit_list(struct rev_info *revs,
diff --git a/transport-helper.c b/transport-helper.c
index 322c722478..e95267a4ab 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1286,6 +1286,8 @@ int transport_helper_init(struct transport *transport, const char *name)
 	if (getenv("GIT_TRANSPORT_HELPER_DEBUG"))
 		debug = 1;
 
+	list_objects_filter_init(&data->transport_options.filter_options);
+
 	transport->data = data;
 	transport->vtable = &vtable;
 	transport->smart_options = &(data->transport_options);
diff --git a/transport.c b/transport.c
index 6ec6130852..a14179684b 100644
--- a/transport.c
+++ b/transport.c
@@ -1113,6 +1113,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		 * will be checked individually in git_connect.
 		 */
 		struct git_transport_data *data = xcalloc(1, sizeof(*data));
+		list_objects_filter_init(&data->options.filter_options);
 		ret->data = data;
 		ret->vtable = &builtin_smart_vtable;
 		ret->smart_options = &(data->options);
diff --git a/upload-pack.c b/upload-pack.c
index 35fe1a3dbb..54583b71c6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -141,6 +141,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->allow_filter_fallback = 1;
 	data->tree_filter_max_depth = ULONG_MAX;
 	packet_writer_init(&data->writer, 1);
+	list_objects_filter_init(&data->filter_options);
 
 	data->keepalive = 5;
 	data->advertise_sid = 0;
-- 
2.37.3.1242.g835d375f85

[PATCH 4/4] list-objects-filter: convert filter_spec to a strbuf

[Jeff King]

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 <peff@peff.net>
---
 list-objects-filter-options.c | 51 +++++++++++++----------------------
 list-objects-filter-options.h |  4 +--
 2 files changed, 20 insertions(+), 35 deletions(-)

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,15 +229,15 @@ 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]));
 	/*
 	 * 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);
 
 /*
-- 
2.37.3.1242.g835d375f85

Re: [PATCH 0/4] list-objects-filter cleanups

[Jeff King]

On Sun, Sep 11, 2022 at 12:57:39AM -0400, Jeff King wrote:

> These patches should be applied on top of jk/plug-list-object-filter-leaks,
> which is currently in next.
> 
>   [1/4]: list-objects-filter: don't memset after releasing filter struct
>   [2/4]: list-objects-filter: handle null default filter spec
>   [3/4]: list-objects-filter: add and use initializers
>   [4/4]: list-objects-filter: convert filter_spec to a strbuf

By the way, there are two small conflicts when merging the result to
next. In ab/submodule-helper-leakfix, some memset() calls got moved to
"{ 0 }" initializers in the variable declarations. The resolution is to
just replace the "{ 0 }" with LIST_OBJECTS_FILTER_INIT (both sides
remove the memset calls).

-Peff

Re: [PATCH 1/4] list-objects-filter: don't memset after releasing filter struct

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we see an error while parsing a "combine" filter, we call
> list_objects_filter_release() to free any allocated memory,
> and then use memset() to return the struct to a known state. But the
> release function already does that reinitializing. Doing it again is
> pointless.

Makes sense.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  list-objects-filter-options.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 6cc4eb8e1c..ea989db260 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -187,10 +187,8 @@ static int parse_combine_filter(
>  
>  cleanup:
>  	strbuf_list_free(subspecs);
> -	if (result) {
> +	if (result)
>  		list_objects_filter_release(filter_options);
> -		memset(filter_options, 0, sizeof(*filter_options));
> -	}
>  	return result;
>  }

Re: [PATCH 2/4] list-objects-filter: handle null default filter spec

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But it's probably worth avoiding this confused state. It's an accident
> waiting to happen, and it will be a problem if we replace the lazy
> initialization from 7e2619d8ff (list_objects_filter_options: plug leak
> of filter_spec strings, 2022-09-08) with a real initialization function.

True.  Thanks for a careful analysis.

Re: [PATCH 4/4] list-objects-filter: convert filter_spec to a strbuf

[Derrick Stolee]

On 9/11/2022 1:03 AM, Jeff King wrote:
> -	if (!filter_options->filter_spec.strdup_strings)
> +	if (!filter_options->filter_spec.buf)
>  		BUG("filter_options not properly initialized");

I couldn't figure out why this would work until I dug into
STRBUF_INIT and found this:

/*
 * Used as the default ->buf value, so that people can always assume
 * buf is non NULL and ->buf is NUL terminated even for a freshly
 * initialized strbuf.
 */
char strbuf_slopbuf[1];

So, this makes sense now.

> -#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRING_LIST_INIT_DUP }
> +#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRBUF_INIT }

And now this macro swap works, too.

Thanks,
-Stolee

Re: [PATCH 0/4] list-objects-filter cleanups

[Derrick Stolee]

On 9/11/2022 12:57 AM, Jeff King wrote:
> In the recent jk/plug-list-object-filter-leaks topic[1], I stopped short
> of fixing all of the callers to actually initialize the filter struct
> beyond zero-ing it.
> 
> This series does the cleanup that I was afraid to do there. :)
> 
> I think the end result is less confusing and error-prone. And as you can
> see in patch 4, it matches how the code originally hoped to be written,
> but the author was also afraid of the zero-initialization thing.
> 
> It is kind of churny, and carries some risk of regression (if I missed a
> spot). IMHO it's worth it, but even if we don't take it, we should pick
> up the first two patches, which are small bug-lets that the conversion
> turned up.

I agree that it's worth it. Hopefully the BUG() you inserted is sufficient
to catch any regression in CI.

Thanks,
-Stolee

Re: [PATCH 4/4] list-objects-filter: convert filter_spec to a strbuf

[Jeff King]

On Mon, Sep 19, 2022 at 01:50:32PM -0400, Derrick Stolee wrote:

> On 9/11/2022 1:03 AM, Jeff King wrote:
> > -	if (!filter_options->filter_spec.strdup_strings)
> > +	if (!filter_options->filter_spec.buf)
> >  		BUG("filter_options not properly initialized");
> 
> I couldn't figure out why this would work until I dug into
> STRBUF_INIT and found this:
> 
> /*
>  * Used as the default ->buf value, so that people can always assume
>  * buf is non NULL and ->buf is NUL terminated even for a freshly
>  * initialized strbuf.
>  */
> char strbuf_slopbuf[1];
> 
> So, this makes sense now.

Yeah. It's a little intimate with the strbuf implementation, but I think
that's OK given the scope of things here. Ironically, if "buf" could be
NULL, then we'd actually be OK with the original zero-initialization. :)

-Peff