Re: [PATCH 0/4] use xmalloc in more places

From: Jeff King <peff@peff.net>
Date: Apr 11, 2019, 19:37 UTC
Message-ID: <20190411193735.GB32528@sigill.intra.peff.net>
In-Reply-To: <20190411191452.GA21290@D-10-18-172-132.dhcp4.washington.edu>

On Thu, Apr 11, 2019 at 12:14:52PM -0700, Taylor Blau wrote:

Show 13 quoted lines

> > This series cleans up most of the bare calls found by:
> >
> >   git grep -E '[^a-z_](m|c|re)alloc\(' '*.c' :^compat :^contrib :^wrapper.c
> 
> This (admittedly pretty awesome) 'git grep' invocation reminds me of a
> series I was pretty sure you wrote to ban functions like 'strcpy' and
> other obviously bad things.
> 
> Some quick searching turned up [1], which landed as f225611d1c
> (automatically ban strcpy(), 2018-07-26). Do we want something similar
> here? Of course, the locations below would have to be exempt, but it
> seems worthwhile (and would save a review cycle in the case that someone
> added a 'malloc' in a patch sent here).

I don't think we can ban malloc, since we have to use it ourselves. :)

With some contortions, we probably could unban it specifically in wrapper.c (though note there are a few other calls I've left which would need to be handled somehow).

Another option would be coccinelle patches to convert malloc() to xmalloc(), etc (with an exception for the wrappers). I'm not entirely comfortable with automatic conversion here because there's often some follow-up adjustments (i.e., we can stop handling allocation errors and maybe delete some code). I think coccinelle can identify callers and barf, though.

I'm not sure whether coccinelle saves review cycles (since that implies people actually run it, though maybe that is better now that it's part of Travis?). It seems to me that it's usually more helpful for people periodically doing follow-up auditing.

So I dunno. If this was a common mistake I'd be more concerned with saving review cycles, but all of the cases I found were actually just leftovers from the very early days of Git.

-Peff