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

[Junio C Hamano <gitster@pobox.com>]

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.

Thanks.