Re: [RFC PATCH 0/5] strvec: add a "nodup" mode, fix memory leaks

From: Jeff King <peff@peff.net>
Date: Dec 17, 2022, 13:13 UTC
Message-ID: <Y53AXmfabIvdkfZz@coredump.intra.peff.net>
In-Reply-To: <RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com>

On Thu, Dec 15, 2022 at 10:11:06AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> This is an alternative to René's [1], his already fixes a leak in "git
> am", and this could be done later, so I'm submitting it as RFC, but it
> could also replace it.
> 
> I think as this series shows extending the "strvec" API to get a
> feature that works like the existing "strdup_strings" that the "struct
> string_list" has can make memory management much simpler.

I know this is kind of a surface level review, but...please don't do this. We have chased so many bugs over the years due to string-list's "maybe this is allocated and maybe not", in both directions (accidental leaks and double-frees).

One of the reasons I advocated for strvec in the first place is so that it would have consistent memory management semantics, at the minor cost of sometimes duplicating them when we don't need to.

And having a nodup form doesn't even save you from having to call strvec_clear(); you still need to do so to avoid leaking the array itself. It only helps in the weird parse-options case, where we don't handle ownership of the array very well (the strvec owns it, but parse-options wants to modify it).

Show 5 quoted lines

> This does make the API slightly more dangerous to use, as it's no
> longer guaranteed that it owns all the members it points to. But as
> the "struct string_list" has shown this isn't an issue in practice,
> and e.g. SANITIZE=address et al are good about finding double-frees,
> or frees of fixed strings.

I would disagree that this hasn't been an issue in practice. A few recent examples:

  - 5eeb9aa208 (refs: fix memory leak when parsing hideRefs config,
    2022-11-17)
  - 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec
    strings, 2022-09-08)
  - 4c81ee9669 (submodule--helper: fix "reference" leak, 2022-09-01)

Now you could argue that those leaks might still exist if we only had a duplicating version of string-list (after all, the problem in a leak is an extra duplication). But IMHO it is the ambiguity and the games we play with setting/unsetting the strdup_strings field that lead to these errors.

And yes, leak-checking and sanitizers can sometimes find these bugs. But that implies triggering the bug in the test suite. And it implies extra time to track and fix them. An interface which is harder to get wrong in the first place is preferable.

-Peff