Re: [PATCH 1/2] add COPY_ARRAY

["René Scharfe" <l.s.r@web.de>]

Am 25.09.2016 um 09:41 schrieb Jeff King:
> 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).

Well, yes, checking if the destination object is big enough requires 
calculating the size of the source object and thus should include a 
check for multiplication overflow already.  Note the word "should". :) 
If that's the case then a smart compiler could elide the duplicate 
check.  Keeping the check in COPY_ARRAY reduces the assumptions we have 
make about its use as well as any previous checks and doesn't cost much.

René