Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984

[Patrick Steinhardt <ps@pks.im>]

On Wed, May 07, 2025 at 01:24:53PM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> Most Git commands use parse_options to parse options, but parse_options
> causes a memory leak when it encounters unknown short options. The leak
> occurs at line 984 in parse-options.c, where git allocates some spaces

Again, no need to quote the exact line number.

> to store the unknown short options and will never release those spaces.
> 
> To address this issue, users can be allowed to provide a custom function
> to allocate memory for unknown options. For example, users can use
> strvec_push to allocate memory for unknown options and later call
> strvec_clear at the end of the Git command to release the memory, thereby
> avoiding the memory leak.

So does that mean that _every_ user of the "parse-options" interfaces
now has to explicitly plug this memory leak when facing unknown options?
That sounds rather undesirable, as there are so many users out there.

> This commit allows users to provide a custom allocation function for
> unknown options via the strdup_fn field in the last struct option. For
> convenience, this commit also introduces a new macro, OPT_UNKNOWN, which
> behaves like OPT_END but takes an additional argument for the allocation
> function. parse_options could get the custom allocation function in struct
> parse_opt_ctx_t by using set_strdup_fn. A simple example to use
> OPT_UNKNOWN is put into t/helper/test-free-unknown-options.c

Hm. Is there any other usecase for the `strdup_fn` field that you can
think about in the future? Otherwise it feels a bit overengineered from
my perspective.

I unfortuntaely don't think the proposed solution works well. To really
plug the memory leak everywhere, we would have to convert every single
user of `parse_options()` to pass `OPTION_UNKNOWN()` instead of
`OPTION_END()`, pass a strvec and clear that strvec at the end. It just
doesn't quite feel like it's worth it to plug a single, bounded memory
leak.

The most important problem that we're facing is that the allocated
string has a lifetime longer than `parse_options()`, because it really
only becomes relevant in the case where `PARSE_OPT_KEEP_UNKNOWN_OPT` is
enabled. If so, the _caller_ of `parse_options()` will want to process
the modified `argv`, and thus they have to be the ones responsible for
freeing potentially-allocated memory once it's unused.

But that'd require us to keep some state. You solve this via the extra
strvec, but that doesn't really look like a great solution to me. An
alternative would be to expose the `parse_options_ctx` and let the
caller eventually call `parse_options_clear()` or something like that.
This would also require us to modify every caller, but the benefit would
be that we now have a single place where we can always allocate memory
in without having to worry about its lifetime.

All of that starts to become kind of involved though. So unless others
disagree with my analysis I just don't think this edge case really is
worth the complexity.

Thanks!

Patrick