Re: [PATCH] clean: do not pass strbuf by value

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jul 30, 2025 at 07:15:28AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Tue, Jul 29, 2025 at 02:03:27PM -0700, Junio C Hamano wrote:
> >> When you pass a structure by value, the callee can modify the
> >> contents of the structure that was passed in without having to worry
> >> about changing the structure the caller has.  Passing structure by
> >
> > s/structure/structures/
> >
> >> value sometimes (but not very often) can be a valid way to give
> >> callee a temporary variable it can freely modify.
> >> 
> >> But not a structure with members that are pointers, like a strbuf.
> >> 
> >> builtin/clean.c:list_and_choose() reads a line interactively from
> >> the user, and passes the line (in a strbuf) to parse_choice() by
> >> value, which then munges by replacing ',' with ' ' (to accept both
> >> comma and space separated list of choices).  But because the strbuf
> >> passed by value still shares the underlying character array buf[],
> >> this ends up munging the caller's strbuf contents.
> >> 
> >> This is a catastrophe waiting to happen.  If the callee causes the
> >> strbuf to be reallocated, the buf[] the caller has will become
> >> dangling, and when the caller does strbuf_release(), it would result
> >> in double-free.
> >> 
> >> Stop calling the function with misleading call-by-value with strbuf.
> >
> > I think the second "with" should be dropped?
> >
> >> 
> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >> ---
> >>  builtin/clean.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > Good finding with an obvious fix. Thanks!
> >
> > Patrick
> 
> "Fix" is a word that is bit stronger than what is actually
> happening, as the code is not yet broken ;-)
> 
> I notice that there are a few structures passed by value in reftable
> (e.g. merged_iter_pqueue in pq.h and string_view in record.h), but I
> only looked at the output of
> 
>   $ git grep '[(,]struct [a-z_]* [^*]*[,)]' \*.h
> 
> and do not know if they are something to worry about.

We originally had a whole lot more, where we passed `struct
reftable_buf` around by value just like we did here. I already got rid
of a bunch of them over time, but we still have calling patterns where
we pass a lot of `struct string_view`s around. Those are all benign,
even though I don't particularly like the calling patterns.

Patrick