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

From: Jeff King <peff@peff.net>
Date: Jul 20, 2018, 00:54 UTC
Message-ID: <20180720005404.GA2179@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kaqgF5zNC0X5+EnuPhYiaav9JiSsgeXF=deryS3sKYq2A@mail.gmail.com>

On Thu, Jul 19, 2018 at 02:47:35PM -0700, Stefan Beller wrote:

Show 9 quoted lines

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

I think we literally had somebody say "I don't have to abide by this in a review because it wasn't in CodingGuidelines." But then, that is perhaps indicative of other problems.

Show 5 quoted lines

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

Ooh, strcat is another one that should be added.

I actually thought about splitting it into three commits (introduce mechanism, then one per function), but it felt like stringing it out. You are probably right, though, that each function deserves its own explanation. And the commit message is already quite long.

Show 8 quoted lines

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

Actually, that's a good point. We've had actual bugs due strcpy(). I can similarly point to bad uses of strcat().

I think I have sufficient fodder for a re-roll along these lines (assuming we like the idea at all; Junio seemed to have some reservations, but I'll reply there separately).

-Peff