Re: [PATCH v1] Travis: also test on 32-bit Linux

From: Jeff King <peff@peff.net>
Date: Mar 5, 2017, 11:36 UTC
Message-ID: <20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net>
In-Reply-To: <282895e1-d9eb-2368-a8e7-8085ad9b17ed@oracle.com>

On Sat, Mar 04, 2017 at 09:08:40PM +0100, Vegard Nossum wrote:

Show 9 quoted lines

> > At a glance, looks like range_set_copy() is using
> > sizeof(struct range_set) == 12, but
> > range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using
> > sizeof(rs->range) == 8.
> 
> Attached patch seems to fix it -- basically, range_set_copy() is trying
> to copy more than it should. It was uncovered with the test case from
> Allan's commit because it's creating enough ranges to overflow the
> initial allocation on 32-bit.

Ugh, yeah, that is definitely a bug.

Show 10 quoted lines

> diff --git a/line-log.c b/line-log.c
> index 951029665..cb0dc1110 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -43,7 +43,7 @@ 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));
> +	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.

I grepped for 'memcpy.*sizeof' and found one other case that's not a bug, but is questionable.

Of the "good" cases, I think most of them could be converted into something more obviously-correct, which would make auditing easier. The three main cases I saw were:

  1. Ones which can probably be converted to COPY_ARRAY().

  2. Ones which just copy a single object, like:

       memcpy(&dst, &src, sizeof(dst));

     Perhaps we should be using struct assignment like:

       dst = src;

     here. It's safer and it should give the compiler more room to
     optimize. The only downside is that if you have pointers, it is
     easy to write "dst = src" when you meant "*dst = *src".

  3. There were a number of alloc-and-copy instances. The copy part is
     the same as (2) above, but you have to repeat the size, which is
     potentially error-prone. I wonder if we would want something like:

       #define ALLOC_COPY(dst, src) do { \
         (dst) = xmalloc(sizeof(*(dst))); \
	 COPY_ARRAY(dst, src, 1); \
       while(0)

     That avoids having to specify the size at all, and triggers a
     compile-time error if "src" and "dst" point to objects of different
     sizes.

     I suspect our friendly neighborhood coccinelle wizards could cook
     up a conversion.

-Peff