On Fri, Nov 29, 2024 at 06:23:45PM +0100, Rubén Justo wrote:
> 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));
>
> Signed-off-by: Rubén Justo <rjusto@xxxxxxxxx>
> ---
>
> 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:
Thanks a lot for fixing this!
> 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);
> + }
Makes sense.
> 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);
> - }
Okay, here we only move existing entries around if the array actually
had entries in the first place. Otherwise there's nothing to move
around. Makes sense.
> + array->nr += (replacement_len - len);
The braces aren't required.
Thanks!
Patrick
