On Wed, Jul 30, 2025 at 07:15:28AM -0700, Junio C Hamano wrote: > 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. 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
