On Wed, May 07, 2025 at 01:24:53PM +0000, Lidong Yan via GitGitGadget wrote: > From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> > > 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
