threads / patch / 63875

patchclean: do not pass strbuf by value

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

## tl;dr

4 messages between Jul 29, 2025 and Jul 31, 2025. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Junio C Hamano· Jul 29, 2025, 21:03 UTC · lore

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 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.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to builtin/clean.c +5 −5
diff --git a/builtin/clean.c b/builtin/clean.c
index 053c94fc6b..224551537e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -477,7 +477,7 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)
  */
 static int parse_choice(struct menu_stuff *menu_stuff,
 			int is_single,
-			struct strbuf input,
+			struct strbuf *input,
 			int **chosen)
 {
 	struct strbuf **choice_list, **ptr;
@@ -485,14 +485,14 @@ static int parse_choice(struct menu_stuff *menu_stuff,
 	int i;
 
 	if (is_single) {
-		choice_list = strbuf_split_max(&input, '\n', 0);
+		choice_list = strbuf_split_max(input, '\n', 0);
 	} else {
-		char *p = input.buf;
+		char *p = input->buf;
 		do {
 			if (*p == ',')
 				*p = ' ';
 		} while (*p++);
-		choice_list = strbuf_split_max(&input, ' ', 0);
+		choice_list = strbuf_split_max(input, ' ', 0);
 	}
 
 	for (ptr = choice_list; *ptr; ptr++) {
@@ -630,7 +630,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
 
 		nr = parse_choice(stuff,
 				  opts->flags & MENU_OPTS_SINGLETON,
-				  choice,
+				  &choice,
 				  &chosen);
 
 		if (opts->flags & MENU_OPTS_SINGLETON) {

-- 
2.50.1-612-g4756c59422
Patrick Steinhardt· Jul 30, 2025, 05:38 UTC · re: Junio C Hamano · lore

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

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/
Show 18 quoted lines
> 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?
Show 5 quoted lines
> 
> 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
Junio C Hamano· Jul 30, 2025, 14:15 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 37 quoted lines
> 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.
Patrick Steinhardt· Jul 31, 2025, 05:31 UTC · re: Junio C Hamano · lore

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

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

← back to recent threads