Re: [PATCH 1/2] introduce "banned function" list

From: Jeff King <peff@peff.net>
Date: Jul 19, 2018, 21:32 UTC
Message-ID: <20180719213222.GB13151@sigill.intra.peff.net>
In-Reply-To: <CAGZ79ka1yHb-485xc-sAK5iVkeyWJq4Q2g-7jzEgHuVUWLBXKg@mail.gmail.com>

On Thu, Jul 19, 2018 at 02:15:54PM -0700, Stefan Beller wrote:

Show 5 quoted lines

> >  Documentation/CodingGuidelines |  3 +++
> 
> I'd prefer to not add more text to our documentation
> (It is already long and in case you run into this problem
> the error message is clear enough IMHO)

I'm fine with that too. I just wondered if somebody would complain in the opposite way: your patch enforces this, but we never made it an "official" guideline.

But that may be overly paranoid. Once upon a time there was some rules lawyering around CodingGuidelines, but I think that was successfully shut down and hasn't reared its head for several years.

Show 12 quoted lines

> > +#define strcpy(x,y) BANNED(strcpy)
> > +
> > +#ifdef HAVE_VARIADIC_MACROS
> 
> In a split second I thought you forgot sprintf that was
> mentioned in the commit message, but then I kept on reading
> just to find it here. I wonder if we want put this #ifdef at the
> beginning of the file (after the guard) as then we can have
> a uncluttered list of banned functions here. The downside is that
> the use of strcpy would not be banned in case you have no
> variadic macros, but we'd still catch it quickly as most people
> have them. Undecided.

Right, exactly. We should catch what we can on lesser platforms, and everything on modern ones. My hope is that people would not generally have to touch this file. I don't think we'll be adding banned functions often.

Show 5 quoted lines

> Regarding the introduction of the functions to this list,
> I would imagine people would find the commit that introduced
> a function to the ban list to look for a reason why.
> Can we include a link[1] to explain why we discourage
> strcpy and sprintf in this commit?

I hoped that it was mostly common knowledge, but that's probably not a good assumption. I agree if there's a good reference, we should link to it.

> [1] https://www.thegeekstuff.com/2013/06/buffer-overflow/?utm_source=feedly
>   This is the best I found. So not sure if it worth it.

Yeah, this one is so-so, because it goes into a lot more detail. I think we can assume that people know that overflowing a buffer is bad. Maybe just a short paragraph like:

  We'll include strcpy() and sprintf() in the initial list of banned
  functions. While these _can_ be used carefully by surrounding them
  with extra code, there's no inherent check that the size of the
  destination buffer is big enough, it's very easy to overflow the
  buffer.

-Peff