Re: [PATCH] line-log: use COPY_ARRAY to fix mis-sized memcpy
On 05/03/2017 12:44, Jeff King wrote:
Show 15 quoted lines
> 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.
Show 21 quoted lines
>
> -- >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>
Show 21 quoted lines
> ---
> 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 Nossum <vegard.nossum@oracle.com>