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

From: Stefan Beller <sbeller@google.com>
Date: Jul 19, 2018, 21:47 UTC
Message-ID: <CAGZ79kaqgF5zNC0X5+EnuPhYiaav9JiSsgeXF=deryS3sKYq2A@mail.gmail.com>
In-Reply-To: <20180719213222.GB13151@sigill.intra.peff.net>

On Thu, Jul 19, 2018 at 2:32 PM Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

>
> On Thu, Jul 19, 2018 at 02:15:54PM -0700, Stefan Beller wrote:
>
> > >  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.

Heh; My preference would be to keep docs as short and concise as possible (and lawyering sounds like "verbosity is a feature" :-P) but still giving advice on controversial (i.e. not enforced by a machine in a deterministic fashion) things such as having braces around one statement for example or leaving them out.

Show 9 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.

In this case I am just an advocate for a better history. Countless times I wanted to know *why* something is the way it is and mailing list and history are not very good at that, as my specific question was not addressed there.

Either it was obvious to all people involved at the time or not written down why that solution (which -- in hindsight -- sucks), was superior to the other solution (which may or may not have been known at the time).

So maybe I would have rather asked, why we start out with these two functions. And you seem to call them "obviously bad", and you take both of them because they need to be handled differently due to the variadic macros. (And those two are "obviously worse" than strcat, as they are used more often. But strcat, being on MS ban list[1], would do just as fine)

(I agree that) Any other function can be added on top with its own commit message on *why* that function. Over time I would expect that the reason is that we got bitten by it, or some other project got famously bitten by it or that some smart people came up with a list[1]. The exception is this one, which basically says: "Here is the mechanism how to do it and $X is the obvious thing to put in first", which I agree with the mechanism as well as that $X seems bad.

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

Now that I am deep down the rathole of discussing a tangent, I just found [2], when searching for "how much common knowledge is wrong", do you know if there is such a list for (CS) security related things?

[2] http://www.marcandangel.com/2008/06/12/60-popular-pieces-of-false-knowledge/

Show 13 quoted lines

>
> > [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.

Sounds good to me, maybe even add "We've had problems with that in the past, see 'git log -S strcpy'", but that may be too much again.

Thanks, Stefan