Patrick Steinhardt <ps@xxxxxx> 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@xxxxxxxxx> >> --- >> 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.
