Re: [PATCH 1/2] add COPY_ARRAY

[Jeff King <peff@peff.net>]

On Sun, Sep 25, 2016 at 09:15:42AM +0200, René Scharfe wrote:

> Add COPY_ARRAY, a safe and convenient helper for copying arrays,
> complementing ALLOC_ARRAY and REALLOC_ARRAY.  Users just specify source,
> destination and the number of elements; the size of an element is
> inferred automatically.

Seems like a fairly readable construct to have.

> It checks if the multiplication of size and element count overflows.
> The inferred size is passed first to st_mult, which allows the division
> there to be done at compilation time.

I wonder if this actually stops any real overflows. My goal with
ALLOC_ARRAY, etc, was to catch these at the malloc stage (which is the
really dangerous part, because we don't want to under-allocate). So the
first hunk of your patch is:

        ALLOC_ARRAY(result, count + 1);
-       memcpy(result, pathspec, count * sizeof(const char *));
+       COPY_ARRAY(result, pathspec, count);

which clearly cannot trigger the st_mult() check, because we would have
done so in the ALLOC_ARRAY call[1].

Other calls are not so obvious, but in general I would expect the
allocation step to be doing this check. If we missed one, then it's
possible that this macro could detect it and prevent a problem. But it
seems like the wrong time to check. The allocation is buggy, and we'd
have to just get lucky to be using COPY_ARRAY(). And I don't even mean
"lucky that we switched to COPY_ARRAY from memcpy for this callsite".
There are lots of sites that allocate and then fill the array one by
one, without ever computing the full size again. So allocation is the
only sensible place to enforce integer overflow.

So I'm not sold on this providing any real integer overflow safety. But
I do otherwise like it, as it drops the extra "sizeof" which has to
repeat either the variable name or the type).

-Peff

[1] Actually, this particular example probably should be using
    st_add(count, 1), though it's likely not a problem in practice
    ("count" is an int, so you cannot easily overflow it back to 0 by
    incrementing 1, though of course overflowing it at all is undefined
    behavior).