Re: [PATCH 1/2] add COPY_ARRAY

> On Sun, Sep 25, 2016 at 09:15:42AM +0200, René Scharfe wrote:
>> 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).