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

From: Stefan Beller <sbeller@google.com>
Date: Jul 13, 2018, 21:10 UTC
Message-ID: <CAGZ79kYpZy4q4vD3z5oZNZYgCYWu4YWeA4DX=E8jFW9=XUe7_g@mail.gmail.com>
In-Reply-To: <20180713210452.GA18909@sigill.intra.peff.net>

On Fri, Jul 13, 2018 at 2:04 PM Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

>
> On Fri, Jul 13, 2018 at 01:58:05PM -0700, Stefan Beller wrote:
>
> > > I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid
> > > that function altogether, even when we _think_ it can't trigger an
> > > overflow. That's easier to reason about (and makes auditing easier).
> >
> > Can we somehow automatically find "bad code" either in pathces
> > or in new code (such as pu), e.g. as a coccicheck for these functions?
>
> 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")"? :-)

Show 7 quoted lines

> 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?

Stefan