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é