Re: [PATCH v3 2/6] string-list: introduce `string_list_setlen()`

From: Jeff King <peff@peff.net>
Date: Apr 25, 2023, 06:21 UTC
Message-ID: <20230425062107.GA4061254@coredump.intra.peff.net>
In-Reply-To: <ae8d0ce1f25f26da09f2e3f5bc68f85cc162ce64.1682374789.git.me@ttaylorr.com>

On Mon, Apr 24, 2023 at 06:20:14PM -0400, Taylor Blau wrote:

Show 12 quoted lines

> However, setting `list->nr` manually is not safe in all instances. There
> are a couple of cases worth worrying about:
> 
>   - If the `string_list` is initialized with `strdup_strings`,
>     truncating the list can lead to overwriting strings which are
>     allocated elsewhere. If there aren't any other pointers to those
>     strings other than the ones inside of the `items` array, they will
>     become unreachable and leak.
> 
>     (We could ourselves free the truncated items between
>     string_list->items[nr] and `list->nr`, but no present or future
>     callers would benefit from this additional complexity).

I wondered how bad it would be to just free those truncated entries when strdup_strings is set. But that led me to another interesting point: the util fields. The regular string_list_clear() will optionally free the util entries, too. We'd potentially need to deal with those, too.

We don't do anything with them here. So code like:

  struct string_list foo = STRING_LIST_INIT_NODUP;

  string_list_append(&foo, "bar")->util = xstrdup("something else");
  string_list_setlen(&foo, 0);

would leak that util field. To be clear, to me this definitely falls under "if it hurts, don't do it", and I think code like above is pretty unlikely. But since the point of our function is to prevent mistakes, I thought it was worth mentioning.

I think we _could_ do something like:

  for (i = nr; i < list->nr; i++) {
	if (list->items[i].util)
		BUG("truncated string list item has non-NULL util field");
  }

though that is technically tighter than we need to be (it could be an unowned util field, after all; we don't know what it means here). So I'm inclined to leave your patch as-is.

This would all be easier if the string_list had a field for "we own the util fields, too" just like it has strdup_strings. Or even a free-ing function. But instead we have ad-hoc solutions like "free_util" and string_list_clear_func(). But that's really outside the scope of your series. </rant> :)

-Peff