Re: [PATCH] strvec: `strvec_splice()` to a statically initialized vector

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> Let's avoid an invalid pointer error in case a client of
> `strvec_splice()` ends up with something similar to:
>
>        struct strvec arr = STRVEC_INIT;
>        const char *rep[] = { "foo" };
>
>        strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));

Well spotted, but the explanation can be a bit more helpful to
casual readers.  If there were a few paragraphs like below

    An empty strvec does not represent the array part of the
    structure with a NULL pointer, but with a singleton empty array,
    to help read-only applications.  This is similar to how an empty
    strbuf uses a singleton empty string.

    This approach requires us to be careful when adding elements to
    an empty instance.  The strvec_splice() API function we recently
    introduced however forgot to special case an empty strvec, and
    ended up applying realloc() to the singleton.
    
before your proposed commit log message, I wouldn't have needed to
go read the implementation of STRVEC_INIT to understand what the fix
is about.  From the fix by itself, it is a bit hard to see why
empty_strvec needs to be special cased, until you re-read the
implementation of STRVEC_INIT.

Thanks.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>
> I've had some time to review the new iteration of the series where
> `strvec_splice()` was introduced and perhaps we want to consider cases
> where we end up using `strvec_splice()` with a statically initialized
> `struct strvec`, i.e:
>
>        struct strvec value = STRVEC_INIT;
>        int s = 0, e = 0;
>
>        ... nothing added to `value` and "s == e == 0" ...
>
>        const char *rep[] = { "foo" };
>        strvec_splice(&arr, s, e, rep, ARRAY_SIZE(rep));
>
>        ... realloc(): invalid pointer
>
> Sorry for getting back to this so late.  This slipped through in my
> review.
>
> I know the series is already in `next`.  To avoid adding noise to the
> series I'm not responding to the conversation, but here is a link to
> it:
>
>   https://lore.kernel.org/git/20241120-b4-pks-leak-fixes-pt10-v3-0-d67f08f45c74@pks.im/
>
>  strvec.c              | 10 ++++++----
>  t/unit-tests/strvec.c | 10 ++++++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/strvec.c b/strvec.c
> index d1cf4e2496..64750e35e3 100644
> --- a/strvec.c
> +++ b/strvec.c
> @@ -61,16 +61,18 @@ void strvec_splice(struct strvec *array, size_t idx, size_t len,
>  {
>  	if (idx + len > array->nr)
>  		BUG("range outside of array boundary");
> -	if (replacement_len > len)
> +	if (replacement_len > len) {
> +		if (array->v == empty_strvec)
> +			array->v = NULL;
>  		ALLOC_GROW(array->v, array->nr + (replacement_len - len) + 1,
>  			   array->alloc);
> +	}
>  	for (size_t i = 0; i < len; i++)
>  		free((char *)array->v[idx + i]);
> -	if (replacement_len != len) {
> +	if ((replacement_len != len) && array->nr)
>  		memmove(array->v + idx + replacement_len, array->v + idx + len,
>  			(array->nr - idx - len + 1) * sizeof(char *));
> -		array->nr += (replacement_len - len);
> -	}
> +	array->nr += (replacement_len - len);
>  	for (size_t i = 0; i < replacement_len; i++)
>  		array->v[idx + i] = xstrdup(replacement[i]);
>  }
> diff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c
> index 855b602337..e66b7bbfae 100644
> --- a/t/unit-tests/strvec.c
> +++ b/t/unit-tests/strvec.c
> @@ -88,6 +88,16 @@ void test_strvec__pushv(void)
>  	strvec_clear(&vec);
>  }
>  
> +void test_strvec__splice_just_initialized_strvec(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	const char *replacement[] = { "foo" };
> +
> +	strvec_splice(&vec, 0, 0, replacement, ARRAY_SIZE(replacement));
> +	check_strvec(&vec, "foo", NULL);
> +	strvec_clear(&vec);
> +}
> +
>  void test_strvec__splice_with_same_size_replacement(void)
>  {
>  	struct strvec vec = STRVEC_INIT;