git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Sep 11, 2022, 05:00 UTC
Message-ID
<Yx1rfYtQAlZGfAak@coredump.intra.peff.net>
In-Reply-To
<Yx1qwlpNbxGEiUgc@coredump.intra.peff.net>

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
Previous: Junio C HamanoNext: Junio C Hamano
Message 4 of 11 in “list-objects-filter cleanups”
  1. 0/4 list-objects-filter cleanupsJeff King, Sep 11, 2022
  2. 1/4 list-objects-filter: don't memset after releasing filter structJeff King, Sep 11, 2022
  3. Junio C HamanoSep 12, 2022
  4. 2/4 list-objects-filter: handle null default filter specJeff King, Sep 11, 2022
  5. Junio C HamanoSep 12, 2022
  6. 3/4 list-objects-filter: add and use initializersJeff King, Sep 11, 2022
  7. 4/4 list-objects-filter: convert filter_spec to a strbufJeff King, Sep 11, 2022
  8. Derrick StoleeSep 19, 2022
  9. Jeff KingSep 19, 2022
  10. Jeff KingSep 11, 2022
  11. Derrick StoleeSep 19, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.