git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH 1/2] introduce "banned function" list
Date: Thu, 19 Jul 2018 21:08:08 -0400	[thread overview]
Message-ID: <20180720010808.GC2179@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqmuumdetr.fsf@gitster-ct.c.googlers.com>

On Thu, Jul 19, 2018 at 03:46:08PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > For enforcement, we can rely on the compiler and just
> > introduce code which breaks compilation when it sees these
> > functions. This has a few advantages:
> >
> >   1. We know it's run as part of a build cycle, so it's
> >      hard to ignore. Whereas an external linter is an extra
> >      step the developer needs to remember to do.
> >
> >   2. Likewise, it's basically free since the compiler is
> >      parsing the code anyway.
> >
> >   3. We know it's robust against false positives (unlike a
> >      grep-based linter).
> >
> > The one big disadvantage is that it will only check code
> > that is actually compiled, so it may miss code that isn't
> > triggered on your particular system. But since presumably
> > people don't add new code without compiling it (and if they
> > do, the banned function list is the least of their worries),
> > we really only care about failing to clean up old code when
> > adding new functions to the list. And that's easy enough to
> > address with a manual audit when adding a new function
> > (which is what I did for the functions here).
> >
> > That leaves only the question of how to trigger the
> > compilation error. The goals are:
> 
> I actually have another question, though.
> 
> Is it a downside that it is cumbersome to override?  This is not a
> rhetorical question.  I am not convinced there will not be a legit
> circumstance that calling strcpy (or whatever we are going to ban)
> is the best solution and it is safe.  By "best", what I mean is "you
> could instead use memcpy/strncpy/whatever" can legitimately be
> argued with "but still using memcpy/strncpy/whatever is inferior
> than using strcpy in this case for such and such reasons".

In my opinion, no, this is not a problem.

My plan is to only add functions which are truly worthless. So with
strcpy(), for example, you _can_ make sure the buffer is big enough
before calling strcpy. But to do so, you by definition must have just
called strlen(), at which point you may as well use memcpy(). It's more
obviously correct, and it's more efficient.

Ditto for sprintf, where you should _always_ be using at least xsnprintf
(or some better tool, depending on the situation).  And for strncpy,
strlcpy (or again, some better tool) is strictly an improvement.  If
these were our functions, I would literally delete them from our
codebase. This is the moral equivalent. ;)

Contrast this with memcpy(). This is on Microsoft's SDL banned list[1],
but I think it's silly for it to be. I would never add it to this list.

Likewise snprintf(). That is a function that _can_ be dangerous due to
unexpected truncation, and I think we should avoid it in general. But I
would not ban it, as it is the correct tool in a few situations (you
have a fixed buffer and returning a truncation error code is fine --
many of our syscall wrappers are in this exact situation). So I would
probably not add it to the ban list (but I feel less strongly than I do
about memcpy).

The nuclear option for overriding is "#undef that-function" in a
particular instance. That covers the rest of the translation unit, but I
think that's probably fine. If we have a function which is mostly
questionable, but we need it sometimes, then we ought to be putting a
sane wrapper around it. And _that_ wrapper can sit in its own file and
#undef as it pleases, keeping the unsafety contained within.

Don't get me wrong, if you have another suggestion that allows easier
one-off overrides, I'm happy to hear it. But I think building this into
the compilation step and having it on-by-default is a huge advantage in
simplicity and having people remember to run it. And I couldn't come up
with a better way to do that.

-Peff

  parent reply	other threads:[~2018-07-20  1:08 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-19 20:33 [PATCH 0/2] fail compilation with strcpy Jeff King
2018-07-19 20:39 ` [PATCH 1/2] introduce "banned function" list Jeff King
2018-07-19 21:11   ` Eric Sunshine
2018-07-19 21:27     ` Jeff King
2018-07-19 21:59       ` Eric Sunshine
2018-07-20  0:55         ` Jeff King
2018-07-19 21:15   ` Stefan Beller
2018-07-19 21:32     ` Jeff King
2018-07-19 21:47       ` Stefan Beller
2018-07-20  0:54         ` Jeff King
2018-07-19 22:46   ` Junio C Hamano
2018-07-19 23:55     ` Randall S. Becker
2018-07-20  1:08     ` Jeff King [this message]
2018-07-20  1:12       ` Jeff King
2018-07-20  9:32       ` Junio C Hamano
2018-07-20 17:45         ` Jeff King
2018-07-20 13:22       ` Theodore Y. Ts'o
2018-07-20 17:56         ` Jeff King
2018-07-20 19:03           ` Junio C Hamano
2018-07-20 12:42   ` Derrick Stolee
2018-07-20 14:41   ` Duy Nguyen
2018-07-20 17:48     ` Elijah Newren
2018-07-20 18:04       ` Jeff King
2018-07-20 18:00     ` Jeff King
2018-07-19 20:39 ` [PATCH 2/2] banned.h: mark strncpy as banned Jeff King
2018-07-19 21:12   ` Ævar Arnfjörð Bjarmason
2018-07-19 21:33     ` Jeff King
2018-07-20 18:58 ` [PATCH 0/2] fail compilation with strcpy Junio C Hamano
2018-07-20 19:18   ` Jeff King
2018-07-20 21:50     ` Junio C Hamano
2018-07-24  9:23 ` [PATCH v2 0/4] " Jeff King
2018-07-24  9:26   ` [PATCH v2 1/4] automatically ban strcpy() Jeff King
2018-07-24 17:20     ` Eric Sunshine
2018-07-26  6:58       ` Jeff King
2018-07-26  7:21         ` [PATCH v3 " Jeff King
2018-07-26 17:33           ` Junio C Hamano
2018-07-27  8:08             ` Jeff King
2018-07-27 17:34               ` Junio C Hamano
2018-07-28  9:24                 ` Jeff King
2018-07-24  9:26   ` [PATCH v2 2/4] banned.h: mark strcat() as banned Jeff King
2018-07-24  9:27   ` [PATCH v2 3/4] banned.h: mark sprintf() " Jeff King
2018-07-24  9:28   ` [PATCH v2 4/4] banned.h: mark strncpy() " Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180720010808.GC2179@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).