From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Subject: [PATCH] clean: do not pass strbuf by value
Date: Tue, 29 Jul 2025 14:03:27 -0700 [thread overview]
Message-ID: <xmqqseieogkg.fsf@gitster.g> (raw)
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(-)
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
next reply other threads:[~2025-07-29 21:03 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-29 21:03 Junio C Hamano [this message]
2025-07-30 5:38 ` [PATCH] clean: do not pass strbuf by value Patrick Steinhardt
2025-07-30 14:15 ` Junio C Hamano
2025-07-31 5:31 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqseieogkg.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).