Re: [PATCH 05/18] convert trivial cases to ALLOC_ARRAY

On Mon, Feb 15, 2016 at 11:32:25PM -0500, Eric Sunshine wrote:

On Mon, Feb 15, 2016 at 11:23 PM, Jeff King [off-list ref] wrote:

quoted

On Mon, Feb 15, 2016 at 11:22:12PM -0500, Eric Sunshine wrote:

quoted

On Mon, Feb 15, 2016 at 4:51 PM, Jeff King [off-list ref] wrote:

quoted

-       path = xmalloc((n+1)*sizeof(char *));
+       ALLOC_ARRAY(path, n+1);

Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
do so here, as well.

Will do. I noticed while going over this before sending it out that it
may also be technically possible for "n+1" to overflow here (and I think
in a few other places in this patch). I don't know how paranoid we want
to be.

Yes, I also noticed those and considered mentioning it. There was also
some multiplication which might be of concern.

    ALLOC_ARRAY(graph->mapping, 2 * graph->column_capacity);

It would be easy enough to manually call st_add() and st_mult() for
those cases, but I haven't examined them closely enough to determine
how likely they would be to overflow, nor do I know if the resulting
noisiness of code is desirable.

Yeah, I'm quite sure that one is safe (we set column_capacity to a fixed
integer immediately beforehand). And many of the "+" ones are likely
safe, too.  If "n" is close to wrapping, then allocating "n" structs
will probably fail beforehand (though not always, if you have a ton of
RAM and "n" is a signed int).

But part of the point of this series is that we shouldn't have to wonder
if things are safe. They should just be obviously so, and we should err
on the side of caution. So I think it probably _is_ worth sprinkling
st_add() calls in those places. I'll take a look for the re-roll.

-Peff