Re: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy

[Vegard Nossum <vegard.nossum@oracle.com>]

On 05/03/2017 12:44, Jeff King wrote:
> On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote:
>
>>>  	range_set_init(dst, src->nr);
>>> -	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
>>> +	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));
>>
>> I think "sizeof(*dst->ranges)" is probably an even better fix, as it
>> infers the type of "dst". But these days we have COPY_ARRAY() to make it
>> even harder to get this kind of thing wrong.
>
> So here's your fix wrapped up with a commit message, mostly for Junio's
> convenience. I listed you as the author, since you did the hard part. If
> you're OK with it, please indicate that it's OK to add your
> signed-off-by. If you prefer to do it differently, feel free to post
> your own patch.

Thanks.

>
> -- >8 --
> From: Vegard Nossum <vegard.nossum@oracle.com>
> Subject: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy
>
> This memcpy meant to get the sizeof a "struct range", not a
> "range_set", as the former is what our array holds. Rather
> than swap out the types, let's convert this site to
> COPY_ARRAY, which avoids the problem entirely (and confirms
> that the src and dst types match).
>
> Note for curiosity's sake that this bug doesn't trigger on
> I32LP64 systems, but does on ILP32 systems. The mistaken
> "struct range_set" has two ints and a pointer. That's 16
> bytes on LP64, or 12 on ILP32. The correct "struct range"
> type has two longs, which is also 16 on LP64, but only 8 on
> ILP32.
>
> Likewise an IL32P64 system would experience the bug.
>
> Signed-off-by: Jeff King <peff@peff.net>

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

> ---
>  line-log.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/line-log.c b/line-log.c
> index 65f3558b3..a2477f601 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -43,9 +43,10 @@ void range_set_release(struct range_set *rs)
>  static void range_set_copy(struct range_set *dst, struct range_set *src)
>  {
>  	range_set_init(dst, src->nr);
> -	memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
> +	COPY_ARRAY(dst->ranges, src->ranges, src->nr);
>  	dst->nr = src->nr;
>  }
> +
>  static void range_set_move(struct range_set *dst, struct range_set *src)
>  {
>  	range_set_release(dst);
>


Vegard