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

From: Jeff King <peff@peff.net>
Date: Jul 20, 2018, 17:56 UTC
Message-ID: <20180720175602.GC22486@sigill.intra.peff.net>
In-Reply-To: <20180720132241.GN30706@thunk.org>

On Fri, Jul 20, 2018 at 09:22:41AM -0400, Theodore Y. Ts'o wrote:

Show 11 quoted lines

> On Thu, Jul 19, 2018 at 09:08:08PM -0400, Jeff King wrote:
> > 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.
> 
> Nitpick: this may be true for git, but it's not strictly true in all
> cases.  I actually have a (non-git) case where strncpy *is* the right
> tool.  And this is one where I'm copying a NUL-terminated string into
> a fixed-length charater array (in the ext4 superblock) which is *not*
> NUL-terminated.  In that case, strncpy works(); but strlcpy() does not
> do what I want.

Thanks for an interesting (and exotic) counter-point. For strcpy(), you always have to run strlen() anyway to know you're not going to overflow, so you might as well memcpy() at that point. But if you really are OK with truncation, then using strncpy() lets you copy while walking over the string only once, which is more efficient. On the other hand, strncpy() fills out NULs to the end of the destination array, so you are paying an extra cost for small strings.

> So I used strncpy() advisedly, and I ignore people running Coccinelle
> scripts and blindly sending patches to "fix" ext4.

Even after hearing your counter-point, I think I'm still OK with the ban. Because as you've noticed, even if the call is fine, it carries an ongoing maintenance cost. Every time you (or somebody else) audits, you have to skip over that known-good call. And you have to deal with well-meaning patches. So to me there's value in eliminating it even if it's an acceptable tool.

We don't have any remaining calls to strncpy() in Git, so this lets us punt on deciding if the ban is worth changing what's there. We can wait for somebody to decide they _really_ need strncpy, and we can discuss it then with a concrete case.

Show 6 quoted lines

> But perhaps that's also a solution for git?  You don't have to
> necessarily put them on a banned list; you could instead have some
> handy, pre-set scripts that scan the entire code base looking for
> "bad" functions with cleanups automatically suggested.  This could be
> run at patch review time, and/or periodically (especially before a
> release).

We do this already with coccinelle for some transformations. But I think there's real value in linting that's always run, and is cheap. Catching these things early means we don't have to spend time on them in review, or dealing with follow-up patches.

-Peff