git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Dec 2, 2024, 12:54 UTC
Message-ID
<Z02t4zPTR6O2Px1n@pks.im>
In-Reply-To
<37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com>
On Fri, Nov 29, 2024 at 06:23:45PM +0100, Rubén Justo wrote:
Show 32 quoted lines
> 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@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:
Thanks a lot for fixing this!
Show 15 quoted lines
> 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.
Show 8 quoted lines
>  	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
Previous: Rubén JustoNext: Rubén Justo
Message 4 of 21 in “strvec: `strvec_splice()` to a statically initialized vector”
  1. strvec: `strvec_splice()` to a statically initialized vectorRubén Justo, Nov 29, 2024
  2. Junio C HamanoDec 2, 2024
  3. Rubén JustoDec 2, 2024
  4. Patrick SteinhardtDec 2, 2024
  5. strvec: `strvec_splice()` to a statically initialized vectorRubén Justo, Dec 3, 2024
  6. Junio C HamanoDec 4, 2024
  7. Rubén JustoDec 4, 2024
  8. Junio C HamanoDec 4, 2024
  9. Rubén JustoDec 4, 2024
  10. Rubén JustoDec 4, 2024
  11. Junio C HamanoDec 4, 2024
  12. Junio C HamanoDec 9, 2024
  13. Junio C HamanoDec 9, 2024
  14. Junio C HamanoDec 9, 2024
  15. Jeff KingDec 9, 2024
  16. Junio C HamanoDec 9, 2024
  17. Rubén JustoDec 9, 2024
  18. karthik nayakDec 4, 2024
  19. Rubén JustoDec 4, 2024
  20. karthik nayakDec 6, 2024
  21. strvec: `strvec_splice()` to a statically initialized vectorRubén Justo, Dec 4, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.