Re: [PATCH] am: don't pass strvec to apply_parse_options()

From: René Scharfe <l.s.r@web.de>
Date: Dec 13, 2022, 18:31 UTC
Message-ID: <2d0d77a4-f6ac-1fa7-bddb-9083579d8dd7@web.de>
In-Reply-To: <221213.86mt7r4ru2.gmgdl@evledraar.gmail.com>

Am 13.12.22 um 09:37 schrieb Ævar Arnfjörð Bjarmason:

Show 60 quoted lines

>
> On Tue, Dec 13 2022, René Scharfe wrote:
>
>> apply_parse_options() passes the array of argument strings to
>> parse_options(), which removes recognized options.  The removed strings
>> are not freed, though.
>>
>> Make a copy of the strvec to pass to the function to retain the pointers
>> of its strings, so we release them all at the end.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  builtin/am.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 30c9b3a9cd..dddf1b9af0 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)
>>  	int res, opts_left;
>>  	int force_apply = 0;
>>  	int options = 0;
>> +	const char **apply_argv;
>>
>>  	if (init_apply_state(&apply_state, the_repository, NULL))
>>  		BUG("init_apply_state() failed");
>> @@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)
>>  	strvec_push(&apply_opts, "apply");
>>  	strvec_pushv(&apply_opts, state->git_apply_opts.v);
>>
>> -	opts_left = apply_parse_options(apply_opts.nr, apply_opts.v,
>> +	/*
>> +	 * Build a copy that apply_parse_options() can rearrange.
>> +	 * apply_opts.v keeps referencing the allocated strings for
>> +	 * strvec_clear() to release.
>> +	 */
>> +	ALLOC_ARRAY(apply_argv, apply_opts.nr);
>> +	COPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);
>> +
>> +	opts_left = apply_parse_options(apply_opts.nr, apply_argv,
>>  					&apply_state, &force_apply, &options,
>>  					NULL);
>>
>> @@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)
>>  	strvec_clear(&apply_paths);
>>  	strvec_clear(&apply_opts);
>>  	clear_apply_state(&apply_state);
>> +	free(apply_argv);
>>
>>  	if (res)
>>  		return res;
>
> I don't mind this going in, but it really feels like a bit of a dirty
> hack.
>
> We have widespread leaks all over the place due to this
> idiom. I.e. parse_options() and a couple of other APIs expect that they
> can munge the "argv", which is fine if it arrives via main(), but not if
> we're editing our own strvecs.

Where? A quick "git grep 'parse_options.*nr'" turns up only this place as one that passes a strvec to parse_options.

Show 6 quoted lines

> I think less of a hack is to teach the eventual parse_options() that
> when it munges it it should free() it. I did that for the revisions API
> in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that
> need free()-ing, 2022-08-02).
>
> What do you think?

Generating string lists and then parsing them is weird. When calls have to cross a process boundary then we have no choice, but in-process we shouldn't have to lower our request to an intermediate text format. git am does it anyway because it writes its options to a file and reads them back when it resumes with --continue, IIUC.

I hope that is and will be the only place that uses parse_options() with a strvec -- and then we don't have to change that function.

If this pattern is used more widely then we could package the copying done by this patch somehow, e.g. by adding a strvec_parse_options() that wraps the real thing.

If we have to change parse_options() at all then I'd prefer it to not free() anything (to keep it usable with main()'s parameters), but to reorder in a non-destructive way. That would mean keeping the NULL sentinel where it is, and making sure all callers use only the returned argc to determine which arguments parse_options() didn't recognize.

René