René Scharfe <l.s.r@xxxxxx> writes:
> parse_options_dup() counts the number of elements in the given array
> without the end marker, allocates enough memory to hold all of them plus
> an end marker, then copies them and terminates the new array. The
> counting part is done by advancing a pointer through the array, and the
> original pointer is reconstructed using pointer subtraction before the
> copy operation.
>
> The function is also prepared to handle a NULL pointer passed to it.
> None of its callers do that currently, but this feature was used by
> 46e91b663b ("checkout: split part of it to new command 'restore'",
> 2019-04-25); it seems worth keeping.
... meaning it was used but since then we rewrote the user and there
is no caller that uses it?
Gee, how are you finding such an instance of use? I am quite
impressed.
> It ends up doing arithmetic on that NULL pointer, though, which is
> undefined in standard C, when it tries to calculate "NULL - 0". Better
> avoid doing that by remembering the originally given pointer value.
>
> There is another issue, though. memcpy(3) does not support NULL
> pointers, even for empty arrays. Use COPY_ARRAY instead, which does
> support such empty arrays. Its call is also shorter and safer by
> inferring the element type automatically.
Nicely explained.
> struct option *parse_options_dup(const struct option *o)
> {
> + const struct option *orig = o;
If I were doing this patch, I'd give the incoming parameter a more
meaningful name and the temporary/local variable that is used to
scan would get/keep the shorter name. IOW
struct parse_options_dup(const struct option *original)
{
const struct option *o = original;
struct option *copied;
int nr = 0;
... loop to count ...
ALLOC_ARRAY(copied, nr + 1);
COPY_ARRAY(copied, original, nr);
...
But your patch is a strict improvement from the current code. Will
queue without any tweak.
Thanks.
> struct option *opts;
> int nr = 0;
>
> while (o && o->type != OPTION_END) {
> nr++;
> o++;
> }
>
> ALLOC_ARRAY(opts, nr + 1);
> - memcpy(opts, o - nr, sizeof(*o) * nr);
> + COPY_ARRAY(opts, orig, nr);
> memset(opts + nr, 0, sizeof(*opts));
> opts[nr].type = OPTION_END;
> return opts;
> }
> --
> 2.24.0
