From: Junio C Hamano <gitster@pobox•com>
To: "Rubén Justo" <rjusto@gmail•com>
Cc: Git List <git@vger•kernel.org>, Patrick Steinhardt <ps@pks•im>
Subject: Re: [PATCH] strvec: `strvec_splice()` to a statically initialized vector
Date: Mon, 02 Dec 2024 10:49:03 +0900 [thread overview]
Message-ID: <xmqqiks2kg6o.fsf@gitster.g> (raw)
In-Reply-To: <37d0abbf-c703-481d-9f26-b237aac54c05@gmail.com> ("Rubén Justo"'s message of "Fri, 29 Nov 2024 18:23:45 +0100")
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;
next prev parent reply other threads:[~2024-12-02 1:49 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-29 17:23 [PATCH] strvec: `strvec_splice()` to a statically initialized vector Rubén Justo
2024-12-02 1:49 ` Junio C Hamano [this message]
2024-12-02 22:01 ` Rubén Justo
2024-12-02 12:54 ` Patrick Steinhardt
2024-12-03 19:47 ` [PATCH v2] " Rubén Justo
2024-12-04 0:09 ` Junio C Hamano
2024-12-04 1:08 ` Rubén Justo
2024-12-04 7:41 ` Junio C Hamano
2024-12-04 8:46 ` Rubén Justo
2024-12-04 8:50 ` Rubén Justo
2024-12-04 10:15 ` Junio C Hamano
2024-12-09 1:32 ` Junio C Hamano
2024-12-09 1:35 ` Junio C Hamano
2024-12-09 1:56 ` Junio C Hamano
2024-12-09 2:15 ` Jeff King
2024-12-09 7:33 ` Junio C Hamano
2024-12-09 22:42 ` Rubén Justo
2024-12-04 11:26 ` karthik nayak
2024-12-04 22:22 ` Rubén Justo
2024-12-06 11:33 ` karthik nayak
2024-12-04 22:44 ` [PATCH v3] " Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqiks2kg6o.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=ps@pks$(echo .)im \
--cc=rjusto@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox