Re: [PATCH v2 02/11] clean: do not pass strbuf by value
- From
Jeff King <peff@peff.net>
- Date
- Aug 2, 2025, 08:38 UTC
- Message-ID
- <20250802083846.GC3711639@coredump.intra.peff.net>
- In-Reply-To
- <20250731225433.4028872-3-gitster@pobox.com>
On Thu, Jul 31, 2025 at 03:54:24PM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
This is definitely an improvement, though I wonder if we could go further.
When I saw the original version of this patch, my first thought was: if we are passing a strbuf that is not modified, then why not just pass a const string pointer? It is more flexible for the caller and makes our intention more clear. (A careful reader will note that this does not pass in the length, so it is different if we expect embedded NULs, but I don't think that is the case here).
But it does not quite work in this case because of this:
Show 13 quoted lines
> 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);
> }We do modify the string! However, that code goes away in your next patch when we are able to switch to a split function that handles multiple delimiters. You do still do an in-place split, but isn't strictly needed.
If we swapped patches 2 and 3 in this series, then the strbuf cleanup can just become:
diff --git a/builtin/clean.c b/builtin/clean.c index a01d130f62..e4b075df70 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -477,17 +477,17 @@ 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, + const char *input, int **chosen) { - struct string_list choice = STRING_LIST_INIT_NODUP; + struct string_list choice = STRING_LIST_INIT_DUP; struct string_list_item *item; int nr = 0; int i; - string_list_split_in_place_f(&choice, input.buf, - is_single ? "\n" : ", ", -1, - STRING_LIST_SPLIT_TRIM); + string_list_split_f(&choice, input, + is_single ? "\n" : ", ", -1, + STRING_LIST_SPLIT_TRIM); for_each_string_list_item(item, &choice) { const char *string; @@ -626,7 +626,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.buf, &chosen); if (opts->flags & MENU_OPTS_SINGLETON) { I dunno. Maybe it is nitpicking, but I think "don't take a strbuf if you only need a string" is a good general rule. Of course there is only one caller here, so flexibility is probably not that important. -Peff