Re: parse-options: avoid arithmetic on pointer that's potentially NULL

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2019, 02:43 UTC
Message-ID: <xmqq5zjocx7i.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <39a0b622-f725-9284-ea50-19cf4078209d@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 11 quoted lines

> 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.

Show 8 quoted lines

> 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.

Show 17 quoted lines

>  	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