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@xxxxxxxxx>
---
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
