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 <redacted>
---
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(-)
From: Patrick Steinhardt <hidden> Date: 2024-12-02 12:54:18
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 <redacted>
---
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:
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.
We use a singleton empty array to initialize a `struct strvec`,
similar to the empty string singleton we use to initialize a `struct
strbuf`.
Note that an empty strvec instance (with zero elements) does not
necessarily need to be an instance initialized with the singleton.
Let's refer to strvec instances initialized with the singleton as
"empty-singleton" instances.
As a side note, this is the current `strvec_pop()`:
void strvec_pop(struct strvec *array)
{
if (!array->nr)
return;
free((char *)array->v[array->nr - 1]);
array->v[array->nr - 1] = NULL;
array->nr--;
}
So, with `strvec_pop()` an instance can become empty but it does
not going to be the an "empty-singleton".
This "empty-singleton" circumstance requires us to be careful when
adding elements to instances. Specifically, when adding the first
element: we detach the strvec instance from the singleton and set the
internal pointer in the instance to NULL. After this point we apply
`realloc()` on the pointer. We do this in `strvec_push_nodup()`, for
example.
The recently introduced `strvec_splice()` API is expected to be
normally used with non-empty strvec's. However, it can also end up
being used with "empty-singleton" strvec's:
struct strvec arr = STRVEC_INIT;
int a = 0, b = 0;
... no modification to arr, a or b ...
const char *rep[] = { "foo" };
strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));
So, we'll try to add elements to an "empty-singleton" strvec instance.
Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
adding a special case for "empty-singleton" strvec's.
Signed-off-by: Rubén Justo <redacted>
---
This iteration adds more detail to the message plus a minor change to
remove some unnecessary parentheses.
Junio: My message in the previous iteration was aimed at readers like
Patrick, who is also the author of `strvec_splice()`. I certainly
assumed too much prior knowledge, which made the review unnecessarily
laborious.
Rereading what I wrote last night, perhaps the problem now is excess.
I hope not. In any case, here it is :-)
Thanks.
strvec.c | 10 ++++++----
t/unit-tests/strvec.c | 10 ++++++++++
2 files changed, 16 insertions(+), 4 deletions(-)
Range-diff against v1:
1: 0b60fcc51a ! 1: c1991e6f3c strvec: `strvec_splice()` to a statically initialized vector
@@ Metadata
## Commit message ##
strvec: `strvec_splice()` to a statically initialized vector
- Let's avoid an invalid pointer error in case a client of
- `strvec_splice()` ends up with something similar to:
+ We use a singleton empty array to initialize a `struct strvec`;
+ similar to the empty string singleton we use to initialize a `struct
+ strbuf`.
+
+ Note that an empty strvec instance (with zero elements) does not
+ necessarily need to be an instance initialized with the singleton.
+ Let's refer to strvec instances initialized with the singleton as
+ "empty-singleton" instances.
+
+ As a side note, this is the current `strvec_pop()`:
+
+ void strvec_pop(struct strvec *array)
+ {
+ if (!array->nr)
+ return;
+ free((char *)array->v[array->nr - 1]);
+ array->v[array->nr - 1] = NULL;
+ array->nr--;
+ }
+
+ So, with `strvec_pop()` an instance can become empty but it does
+ not going to be the an "empty-singleton".
+
+ This "empty-singleton" circumstance requires us to be careful when
+ adding elements to instances. Specifically, when adding the first
+ element: when we detach the strvec instance from the singleton and
+ set the internal pointer in the instance to NULL. After this point we
+ apply `realloc()` on the pointer. We do this in
+ `strvec_push_nodup()`, for example.
+
+ The recently introduced `strvec_splice()` API is expected to be
+ normally used with non-empty strvec's. However, it can also end up
+ being used with "empty-singleton" strvec's:
struct strvec arr = STRVEC_INIT;
+ int a = 0, b = 0;
+
+ ... no modification to arr, a or b ...
+
const char *rep[] = { "foo" };
+ strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));
+
+ So, we'll try to add elements to an "empty-singleton" strvec instance.
- strvec_splice(&arr, 0, 0, rep, ARRAY_SIZE(rep));
+ Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
+ adding a special case for strvec's initialized with the singleton.
Signed-off-by: Rubén Justo [off-list ref]
@@ strvec.c: void strvec_splice(struct strvec *array, size_t idx, size_t len,
(array->nr - idx - len + 1) * sizeof(char *));
- array->nr += (replacement_len - len);
- }
-+ 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]);
}
--
2.47.0.281.g7eb946317c
Rubén Justo [off-list ref] writes:
Nit: Is the commit subject missing a verb?
We use a singleton empty array to initialize a `struct strvec`,
similar to the empty string singleton we use to initialize a `struct
strbuf`.
So a singleton empty array is a statically allocated array element, so
for strvec, this would be `const char *empty_strvec[] = { NULL }`.
Note that an empty strvec instance (with zero elements) does not
necessarily need to be an instance initialized with the singleton.
Let's refer to strvec instances initialized with the singleton as
"empty-singleton" instances.
Right, so when we add elements ideally, we ideally check whether it is a
singleton or not. This is evident in `strvec_push_nodup()`:
void strvec_push_nodup(struct strvec *array, char *value)
{
if (array->v == empty_strvec)
array->v = NULL;
ALLOC_GROW(array->v, array->nr + 2, array->alloc);
array->v[array->nr++] = value;
array->v[array->nr] = NULL;
}
As a side note, this is the current `strvec_pop()`:
void strvec_pop(struct strvec *array)
{
if (!array->nr)
return;
free((char *)array->v[array->nr - 1]);
array->v[array->nr - 1] = NULL;
array->nr--;
}
So, with `strvec_pop()` an instance can become empty but it does
not going to be the an "empty-singleton".
Correct, since we simply set the array element to NULL, but this is
still a dynamically allocated array.
Nit: The sentence reads a bit weirdly.
This "empty-singleton" circumstance requires us to be careful when
adding elements to instances. Specifically, when adding the first
element: we detach the strvec instance from the singleton and set the
internal pointer in the instance to NULL. After this point we apply
`realloc()` on the pointer. We do this in `strvec_push_nodup()`, for
example.
The recently introduced `strvec_splice()` API is expected to be
normally used with non-empty strvec's. However, it can also end up
being used with "empty-singleton" strvec's:
struct strvec arr = STRVEC_INIT;
int a = 0, b = 0;
... no modification to arr, a or b ...
const char *rep[] = { "foo" };
strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));
So, we'll try to add elements to an "empty-singleton" strvec instance.
Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
adding a special case for "empty-singleton" strvec's.
So everything said here makes sense, that's a great explanation.
Signed-off-by: Rubén Justo <redacted>
---
This iteration adds more detail to the message plus a minor change to
remove some unnecessary parentheses.
Junio: My message in the previous iteration was aimed at readers like
Patrick, who is also the author of `strvec_splice()`. I certainly
assumed too much prior knowledge, which made the review unnecessarily
laborious.
Rereading what I wrote last night, perhaps the problem now is excess.
I hope not. In any case, here it is :-)
I would say this is very useful over the first iteration, considering I
am someone without prior knowledge here.
On Wed, Dec 04, 2024 at 11:26:27AM +0000, karthik nayak wrote:
Nit: Is the commit subject missing a verb?
I guess something like "To strvec_splice" sounded good in my head
:)
quoted
We use a singleton empty array to initialize a `struct strvec`,
similar to the empty string singleton we use to initialize a `struct
strbuf`.
So a singleton empty array is a statically allocated array element, so
for strvec, this would be `const char *empty_strvec[] = { NULL }`.
quoted
Note that an empty strvec instance (with zero elements) does not
necessarily need to be an instance initialized with the singleton.
Let's refer to strvec instances initialized with the singleton as
"empty-singleton" instances.
Right, so when we add elements ideally, we ideally check whether it is a
singleton or not. This is evident in `strvec_push_nodup()`:
void strvec_push_nodup(struct strvec *array, char *value)
{
if (array->v == empty_strvec)
array->v = NULL;
ALLOC_GROW(array->v, array->nr + 2, array->alloc);
array->v[array->nr++] = value;
array->v[array->nr] = NULL;
}
quoted
As a side note, this is the current `strvec_pop()`:
void strvec_pop(struct strvec *array)
{
if (!array->nr)
return;
free((char *)array->v[array->nr - 1]);
array->v[array->nr - 1] = NULL;
array->nr--;
}
So, with `strvec_pop()` an instance can become empty but it does
not going to be the an "empty-singleton".
Correct, since we simply set the array element to NULL, but this is
still a dynamically allocated array.
Nit: The sentence reads a bit weirdly.
quoted
This "empty-singleton" circumstance requires us to be careful when
adding elements to instances. Specifically, when adding the first
element: we detach the strvec instance from the singleton and set the
internal pointer in the instance to NULL. After this point we apply
`realloc()` on the pointer. We do this in `strvec_push_nodup()`, for
example.
The recently introduced `strvec_splice()` API is expected to be
normally used with non-empty strvec's. However, it can also end up
being used with "empty-singleton" strvec's:
struct strvec arr = STRVEC_INIT;
int a = 0, b = 0;
... no modification to arr, a or b ...
const char *rep[] = { "foo" };
strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));
So, we'll try to add elements to an "empty-singleton" strvec instance.
Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
adding a special case for "empty-singleton" strvec's.
So everything said here makes sense, that's a great explanation.
Thanks.
quoted
Signed-off-by: Rubén Justo <redacted>
---
This iteration adds more detail to the message plus a minor change to
remove some unnecessary parentheses.
Junio: My message in the previous iteration was aimed at readers like
Patrick, who is also the author of `strvec_splice()`. I certainly
assumed too much prior knowledge, which made the review unnecessarily
laborious.
Rereading what I wrote last night, perhaps the problem now is excess.
I hope not. In any case, here it is :-)
I would say this is very useful over the first iteration, considering I
am someone without prior knowledge here.
I'm glad to read that. I guess Junio is to blame ;) Thanks.
Why is this second block of changes needed? Will array-nr ever be 0 when
we reach here?
I'm not sure I understand your questions.
At that point, `array->nr` is the initial number of entries in the
vector. It can be 0 when `strvec_splice()` is applied to an empty
vector.
We are moving the line where we update "array->nr" outside the `if`
block because we want to do it even when we are not moving existing
entries. Again, this happens when `strvec_splice()` is applied to an
empty vector.
Finally, we don't mind too much (or value more the simplicity) of the
now unconditional update of "array->nr" because a clever compiler will
give us the third arm of the if: "else -> do nothing". When
`replacement_len == len` => "array->nr += 0" => do nothing.
quoted
for (size_t i = 0; i < replacement_len; i++)
array->v[idx + i] = xstrdup(replacement[i]);
}
We use a singleton empty array to initialize a `struct strvec`;
similar to the empty string singleton we use to initialize a `struct
strbuf`.
Note that an empty strvec instance (with zero elements) does not
necessarily need to be an instance initialized with the singleton.
Let's refer to strvec instances initialized with the singleton as
"empty-singleton" instances.
As a side note, this is the current `strvec_pop()`:
void strvec_pop(struct strvec *array)
{
if (!array->nr)
return;
free((char *)array->v[array->nr - 1]);
array->v[array->nr - 1] = NULL;
array->nr--;
}
So, with `strvec_pop()` an instance can become empty but it does
not going to be the an "empty-singleton".
This "empty-singleton" circumstance requires us to be careful when
adding elements to instances. Specifically, when adding the first
element: when we detach the strvec instance from the singleton and
set the internal pointer in the instance to NULL. After this point we
apply `realloc()` on the pointer. We do this in
`strvec_push_nodup()`, for example.
The recently introduced `strvec_splice()` API is expected to be
normally used with non-empty strvec's. However, it can also end up
being used with "empty-singleton" strvec's:
struct strvec arr = STRVEC_INIT;
int a = 0, b = 0;
... no modification to arr, a or b ...
const char *rep[] = { "foo" };
strvec_splice(&arr, a, b, rep, ARRAY_SIZE(rep));
So, we'll try to add elements to an "empty-singleton" strvec instance.
Avoid misapplying `realloc()` to the singleton in `strvec_splice()` by
adding a special case for strvec's initialized with the singleton.
Signed-off-by: Rubén Justo <redacted>
---
This iteration fixes a problem we saw when running with SANITIZE=leak.
Although it wasn't a leak.
We need to end the array because `realloc(NULL)` is not going to give
us that { NULL }. I know it's something I considered at some point
because I thought about a change like `CALLOC_GROW()`. Perhaps
another time.
strvec.c | 11 +++++++----
t/unit-tests/strvec.c | 10 ++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
Why is this second block of changes needed? Will array-nr ever be 0 when
we reach here?
I'm not sure I understand your questions.
At that point, `array->nr` is the initial number of entries in the
vector. It can be 0 when `strvec_splice()` is applied to an empty
vector.
We are moving the line where we update "array->nr" outside the `if`
block because we want to do it even when we are not moving existing
entries. Again, this happens when `strvec_splice()` is applied to an
empty vector.
Ah. I was considering that ALLOC_GROW would update `array->nr`, but it
doesn't. So you're right.
Finally, we don't mind too much (or value more the simplicity) of the
now unconditional update of "array->nr" because a clever compiler will
give us the third arm of the if: "else -> do nothing". When
`replacement_len == len` => "array->nr += 0" => do nothing.