On Tue, Mar 11, 2025 at 10:19:55PM -0400, Peijian Ju wrote:
> > BTW, this strstr() isn't quite sufficient to prevent problems, as it
> > would not find placeholders which _do_ exist but which aren't handled.
> > One of the first things I tried was:
> >
> > git cat-file --batch-command='%(objecttype) %(objectsize)'
> >
> > and feeding it "remote-object-info /path/to/repo some-oid". And it
> > segfaulted.
> >
> > -Peff
>
> Thank you, Peff. Yes, you are right. It is a bug. I am adding a new
> logic in v12:
> 1. Iterating on the `opt->format` to see if there are any unsupported
> placeholders. If there is, error with unspported placeholders.
> 2. Adding more test cases to cover different formats, e.g., just
> `%(objectsize)`, just `%(objectname)`, mixed usage of supported and
> unsupported placeholders.
Yes, though it would be nice for step 1 to avoid re-parsing the string.
I think you could either:
1. After the mark_query pass in batch_objects(), check for unsupported
pointers in expand_data. The downside here is that you'd have to
match each one that you _don't_ allow (so if somebody adds a new
one and forgets to update your list, it wouldn't be caught).
2. In expand_atom() or expand_format(), check an allow-list using
is_atom(), when remote-mode is in use. The downside here is that I
think we'd eventually want to move that parsing and formatting to
the shared ref-filter API. But maybe that API could provide some
kind of "check that this atom is allowed" function pointer.
I do wonder if there might be a way to also just notice that we don't
have the requested information and handle it gracefully. I didn't
reproduce it again just now, but I'd guess the segfault is due to
feeding garbage to type_name() in expand_atom().
So maybe if we initialized expand_data fully (so that data->type is
always OBJ_BAD or something) and then checked for a NULL return from
type_name(), we could do something sensible in expand_atom(), like
insert a blank string or similar. And then it is not an error to ask for
%(objecttype), but you will just not get useful data for those entries.
