On Thu, Jul 19, 2018 at 1:39 PM Jeff King <peff@xxxxxxxx> wrote: > > There are a few standard C functions (like strcpy) which are > easy to misuse. We generally discourage these in reviews, > but we haven't put advice in CodingGuidelines, nor provided > any automated enforcement. The latter is especially > important because it's more consistent, and it can often > save a round-trip of review. Thanks for writing these patches. I would think this is a good idea to have as I certainly would use banned functions if not told by the compiler not to. > 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) > @@ -0,0 +1,19 @@ > +#ifndef BANNED_H > +#define BANNED_H > + > +/* > + * This header lists functions that have been banned from our code base, > + * because they're too easy to misuse (and even if used correctly, > + * complicate audits). Including this header turns them into compile-time > + * errors. > + */ > + > +#define BANNED(func) sorry_##func##_is_a_banned_function() > + > +#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. 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? [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. Thanks, Stefan
