Re: [PATCH] blame: prefer xsnprintf to strcpy for colors

[Jeff King <peff@peff.net>]

On Fri, Jul 13, 2018 at 02:10:24PM -0700, Stefan Beller wrote:

> > I'd be happy to declare strcpy() totally banned (and it more or less
> > is). I found this with a simple "git grep", though it seems like a
> > trivial application of coccinelle to find it. The question is what to
> > convert it into.
> 
> into some "meta BUG("your review process failed")"? :-)

Heh, yes, I was tempted to suggest that.

> > xsnprintf() is often a good choice, but not always
> > (e.g., if the destination isn't an array, we'd have to get the size from
> > somewhere else).
> >
> > I wouldn't be surprised if there's a way to ask coccinelle to convert
> > the easy cases and barf with an error on the hard cases or something. I
> > don't know the tool very well.
> 
> I was just suggesting that tool as it is run on pu by some automation,
> hence it would not fall through the cracks. I mean how often to you
> happen to run git grep looking for strcpy on our code base and do we
> want to rely on that in the long run?

Clearly not often enough. :)

I probably do it once or twice a year, but the ideal is "as part of
testing every topic". It wouldn't be hard to script around "git grep" as
part of the DEVELOPER build), but I think that still turns up some false
positives. Not for strcpy() or sprintf(), but snprintf() for example is
easy to use badly but sometimes the correct tool. We also have an
strncpy() which would be easy to turn into memcpy(), but it's in the
compat/regex code, which preferably we wouldn't change.

There are also probably better tools than grep (i.e., that actually
parse C), but they may not be worth the overhead (though if we can reuse
cocci for this, that seems easy enough).

As an aside, I recently got introduced to Microsoft's SDL Banned
Function list:

  https://msdn.microsoft.com/en-us/library/bb288454.aspx

They even hate memcpy! I'll grant that you _can_ use memcpy badly, but
it's often the right tool (and IMHO the C11 Annex.K memcpy_s() is not
much better).

-Peff