On Mon, Apr 24, 2023 at 06:20:14PM -0400, Taylor Blau wrote:
> 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
