Am 13.12.22 um 09:37 schrieb Ævar Arnfjörð Bjarmason:
>
> 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@xxxxxx>
>> ---
>> 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.
> 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é
