{"thread":{"id":"48922","subject":"[PATCH 0/2] fail compilation with strcpy","startedAt":"2018-07-19T20:33:03Z","lastAt":"2018-07-28T09:24:47Z","messageCount":42,"participants":["Jeff King","Eric Sunshine","Ævar Arnfjörð Bjarmason","Stefan Beller","Junio C Hamano","Randall S. Becker","Derrick Stolee","Theodore Y. Ts'o","Duy Nguyen","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"353075","messageId":"20180719203259.GA7869@sigill.intra.peff.net","threadId":"48922","inReplyTo":null,"subject":"[PATCH 0/2] fail compilation with strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-19T20:33:00Z","receivedAt":"2018-07-19T20:33:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a patch series to address the discussion in the thread at:\n\n https://public-inbox.org/git/20180713204350.GA16999@sigill.intra.peff.net/\n\nBasically, the question was: can we declare strcpy banned and have a\nlinter save us the trouble of finding it in review. The answer is yes,\nthe compiler is good at that. ;)\n\nThere are probably as many lists of banned functions as there are coding\nstyle documents. I don't agree with every entry in the ones I've seen.\nAnd in many cases coccinelle is a better choice, because the problem is\nnot \"this function is so bad your patch should not even make it to the\nlist with it\", but \"don't do it like A; we prefer to do it like B\ninstead\". And coccinelle does the latter more flexibly and\nautomatically.\n\nSo I tried to pick some obvious and uncontroversial candidates here.\ngets() could be another one, but it's mostly banned already (it's out of\nthe standard, and most libcs mark it with a deprecated attribute).\n\nNote that this needs to be applied on top of 022d2ac1f3 (blame: prefer\nxsnprintf to strcpy for colors, 2018-07-13) or it will complain loudly. :)\n\n [1/2]: introduce \"banned function\" list\n [2/2]: banned.h: mark strncpy as banned\n\n Documentation/CodingGuidelines | 3 +++\n banned.h | 20 ++++++++++++++++++++\n git-compat-util.h | 2 ++\n 3 files changed, 25 insertions(+)\n create mode 100644 banned.h\n\n-Peff\n"},{"id":"353076","messageId":"20180719203901.GA8079@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180719203259.GA7869@sigill.intra.peff.net","subject":"[PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-19T20:39:01Z","receivedAt":"2018-07-19T20:39:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few standard C functions (like strcpy) which are\neasy to misuse. We generally discourage these in reviews,\nbut we haven't put advice in CodingGuidelines, nor provided\nany automated enforcement. The latter is especially\nimportant because it's more consistent, and it can often\nsave a round-trip of review.\n\nLet's start by banning strcpy() and sprintf(). It's not\nimpossible to use these correctly, but it's easy to do so\nincorrectly, and there's always a better option.\n\nFor enforcement, we can rely on the compiler and just\nintroduce code which breaks compilation when it sees these\nfunctions. This has a few advantages:\n\n 1. We know it's run as part of a build cycle, so it's\n hard to ignore. Whereas an external linter is an extra\n step the developer needs to remember to do.\n\n 2. Likewise, it's basically free since the compiler is\n parsing the code anyway.\n\n 3. We know it's robust against false positives (unlike a\n grep-based linter).\n\nThe one big disadvantage is that it will only check code\nthat is actually compiled, so it may miss code that isn't\ntriggered on your particular system. But since presumably\npeople don't add new code without compiling it (and if they\ndo, the banned function list is the least of their worries),\nwe really only care about failing to clean up old code when\nadding new functions to the list. And that's easy enough to\naddress with a manual audit when adding a new function\n(which is what I did for the functions here).\n\nThat leaves only the question of how to trigger the\ncompilation error. The goals are:\n\n - it should be portable; ideally this would trigger\n everywhere, and does not need to be part of a DEVELOPER=1\n setup (because unlike warnings which may depend on the\n compiler or system, this is a clear indicator of\n something wrong in the code).\n\n - it should generate a readable error that gives the\n developer a clue what happened\n\n - it should avoid generating too much other cruft that\n makes it hard to see the actual error\n\n - it should mention the original callsite in the error\n\nThe technique used here is to convert the banned function\ninto a call to a descriptive but non-existent function. The\noutput from gcc 7 looks like this (on a checkout without\n022d2ac1f3, which removes the final strcpy from blame.c):\n\n CC builtin/blame.o\n In file included from ./git-compat-util.h:1242:0,\n from ./cache.h:4,\n from builtin/blame.c:8:\n builtin/blame.c: In function ‘cmd_blame’:\n ./banned.h:11:22: error: implicit declaration of function ‘sorry_strcpy_is_a_banned_function’ [-Werror=implicit-function-declaration]\n #define BANNED(func) sorry_##func##_is_a_banned_function()\n ^\n ./banned.h:13:21: note: in expansion of macro ‘BANNED’\n #define strcpy(x,y) BANNED(strcpy)\n ^~~~~~\n builtin/blame.c:1074:4: note: in expansion of macro ‘strcpy’\n strcpy(repeated_meta_color, GIT_COLOR_CYAN);\n ^~~~~~\n\nThis pretty clearly shows the original callsite along with\nthe descriptive function name, and it mentions banned.h,\nwhich contains more information.\n\nWhat's not perfectly ideal is:\n\n 1. We'll only trigger with -Wimplicit-function-declaration\n (and only stop compilation with -Werror). These are\n generally enabled by DEVELOPER=1. If you _don't_ have\n that set, we'll still catch the problem, but only at\n link-time, with a slightly less useful message:\n\n /usr/bin/ld: builtin/blame.o: in function `cmd_blame':\n /home/peff/tmp/builtin/blame.c:1074: undefined reference to `sorry_strcpy_is_a_banned_function'\n collect2: error: ld returned 1 exit status\n\n If instead we convert this to a reference to an\n undefined variable, that always dies immediately. But\n gcc seems to print the set of errors twice, which\n clutters things up.\n\n 2. The expansion through BANNED() adds an extra layer of\n error. Curiously, though, removing it (and just\n expanding strcpy directly to the bogus function name)\n causes gcc _not_ to report the original line of code.\n\nSo experimentally, this seems to behave pretty well on gcc.\nClang looks OK, too, though:\n\n - it actually produces a better message for the undeclared\n identifier than for the implicit function (it doesn't\n double the errors, and for the implicit function it\n actually produces an extra complaint about\n strict-prototypes).\n\n - the BANNED indirection has no effect (it shows the\n original line of code either way)\n\nSo if we want to optimize for clang's output, we could\nswitch both of those decisions.\n\nSigned-off-by: Jeff King \n---\nSo you might guess that I experimented mostly with gcc\n(which is what I normally use), and then just double-checked\nclang at the end. After seeing those results, I actually\nthink clang does a better job of producing good error\nmessages overall, and perhaps we should not cater to gcc. I\ndunno. It probably doesn't matter that much either way.\n\nI'd also be happy if somebody showed an alternative that\nis even cleaner. Sadly I don't think you can trigger an\n#error from the _expansion_ of a macro.\n\n Documentation/CodingGuidelines | 3 +++\n banned.h | 19 +++++++++++++++++++\n git-compat-util.h | 2 ++\n 3 files changed, 24 insertions(+)\n create mode 100644 banned.h\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 48aa4edfbd..232f17becd 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -391,6 +391,9 @@ For C programs:\n must be declared with \"extern\" in header files. However, function\n declarations should not use \"extern\", as that is already the default.\n \n+ - A few easy-to-misuse functions like `strcpy` are banned; see\n+ `banned.h` for the complete list.\n+\n For Perl programs:\n \n - Most of the C guidelines above apply.\ndiff --git a/banned.h b/banned.h\nnew file mode 100644\nindex 0000000000..fe81020e0f\n--- /dev/null\n+++ b/banned.h\n@@ -0,0 +1,19 @@\n+#ifndef BANNED_H\n+#define BANNED_H\n+\n+/*\n+ * This header lists functions that have been banned from our code base,\n+ * because they're too easy to misuse (and even if used correctly,\n+ * complicate audits). Including this header turns them into compile-time\n+ * errors.\n+ */\n+\n+#define BANNED(func) sorry_##func##_is_a_banned_function()\n+\n+#define strcpy(x,y) BANNED(strcpy)\n+\n+#ifdef HAVE_VARIADIC_MACROS\n+#define sprintf(...) BANNED(sprintf)\n+#endif\n+\n+#endif /* BANNED_H */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9a64998b24..51bece30ac 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1239,4 +1239,6 @@ extern void unleak_memory(const void *ptr, size_t len);\n #define UNLEAK(var) do {} while (0)\n #endif\n \n+#include \"banned.h\"\n+\n #endif\n-- \n2.18.0.540.g6c38643a7b\n\n"},{"id":"353077","messageId":"20180719203934.GB8079@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180719203259.GA7869@sigill.intra.peff.net","subject":"[PATCH 2/2] banned.h: mark strncpy as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-19T20:39:34Z","receivedAt":"2018-07-19T20:39:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The strncpy() function is less horrible than strcpy(). But\nit's still pretty easy to misuse because of its funny\ntermination semantics. And we already have a ready-made\nalternative in strlcpy. So let's ban it, to make sure uses\ndon't creep in.\n\nNote that there is one instance of strncpy in\ncompat/regex/regcomp.c. But this doesn't trigger the\nban-list even when compiling with NO_REGEX=1, because we\ndon't use git-compat-util.h when compiling it (instead we\nrely on the system includes from the upstream library).\n\nSince this use of strncpy was verified by manual inspection\nand since it doesn't trigger the automated ban-list, we're\nbetter off leaving it to keep our divergence from glibc\nminimal.\n\nSigned-off-by: Jeff King \n---\n banned.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/banned.h b/banned.h\nindex fe81020e0f..ae6aaaa4a9 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -11,6 +11,7 @@\n #define BANNED(func) sorry_##func##_is_a_banned_function()\n \n #define strcpy(x,y) BANNED(strcpy)\n+#define strncpy(x,y,n) BANNED(strncpy)\n \n #ifdef HAVE_VARIADIC_MACROS\n #define sprintf(...) BANNED(sprintf)\n-- \n2.18.0.540.g6c38643a7b\n"},{"id":"353081","messageId":"CAPig+cSErDoZ5XFjfJsqfEKZ6PSKfOos=9HW-7FDXU1XEtuktA@mail.gmail.com","threadId":"48922","inReplyTo":"20180719203901.GA8079@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-19T21:11:15Z","receivedAt":"2018-07-19T21:11:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 19, 2018 at 4:39 PM Jeff King wrote:\n> [...]\n> Let's start by banning strcpy() and sprintf(). It's not\n> impossible to use these correctly, but it's easy to do so\n> incorrectly, and there's always a better option.\n> [...]\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/banned.h b/banned.h\n> @@ -0,0 +1,19 @@\n> +/*\n> + * This header lists functions that have been banned from our code base,\n> + * because they're too easy to misuse (and even if used correctly,\n> + * complicate audits). Including this header turns them into compile-time\n> + * errors.\n> + */\n\nWhen the above talks about \"including this header\", the implication is\nthat it must be included _after_ the system header(s) which declare\nthe banned functions. I wonder if that requirement should be stated\nhere explicitly.\n\n(Probably not worth a re-roll.)\n\n> +#define BANNED(func) sorry_##func##_is_a_banned_function()\n> +\n> +#define strcpy(x,y) BANNED(strcpy)\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> @@ -1239,4 +1239,6 @@ extern void unleak_memory(const void *ptr, size_t len);\n> #define UNLEAK(var) do {} while (0)\n> #endif\n>\n> +#include \"banned.h\"\n"},{"id":"353082","messageId":"87y3e7aq0e.fsf@evledraar.gmail.com","threadId":"48922","inReplyTo":"20180719203934.GB8079@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] banned.h: mark strncpy as banned","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-19T21:12:49Z","receivedAt":"2018-07-19T21:12:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 19 2018, Jeff King wrote:\n\n> Since this use of strncpy was verified by manual inspection\n> and since it doesn't trigger the automated ban-list, we're\n> better off leaving it to keep our divergence from glibc\n> minimal.\n\nFWIW it's s/glibc/gawk/. It's originally from glibc, but gawk\nperma-forked it long ago, and an ancient copy of thath is the one we\nuse.\n"},{"id":"353083","messageId":"CAGZ79ka1yHb-485xc-sAK5iVkeyWJq4Q2g-7jzEgHuVUWLBXKg@mail.gmail.com","threadId":"48922","inReplyTo":"20180719203901.GA8079@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-19T21:15:54Z","receivedAt":"2018-07-19T21:16:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 19, 2018 at 1:39 PM Jeff King wrote:\n>\n> There are a few standard C functions (like strcpy) which are\n> easy to misuse. We generally discourage these in reviews,\n> but we haven't put advice in CodingGuidelines, nor provided\n> any automated enforcement. The latter is especially\n> important because it's more consistent, and it can often\n> save a round-trip of review.\n\nThanks for writing these patches. I would think this is a\ngood idea to have as I certainly would use banned functions\nif not told by the compiler not to.\n\n> Documentation/CodingGuidelines | 3 +++\n\nI'd prefer to not add more text to our documentation\n(It is already long and in case you run into this problem\nthe error message is clear enough IMHO)\n\n\n> @@ -0,0 +1,19 @@\n> +#ifndef BANNED_H\n> +#define BANNED_H\n> +\n> +/*\n> + * This header lists functions that have been banned from our code base,\n> + * because they're too easy to misuse (and even if used correctly,\n> + * complicate audits). Including this header turns them into compile-time\n> + * errors.\n> + */\n> +\n> +#define BANNED(func) sorry_##func##_is_a_banned_function()\n> +\n> +#define strcpy(x,y) BANNED(strcpy)\n> +\n> +#ifdef HAVE_VARIADIC_MACROS\n\nIn a split second I thought you forgot sprintf that was\nmentioned in the commit message, but then I kept on reading\njust to find it here. I wonder if we want put this #ifdef at the\nbeginning of the file (after the guard) as then we can have\na uncluttered list of banned functions here. The downside is that\nthe use of strcpy would not be banned in case you have no\nvariadic macros, but we'd still catch it quickly as most people\nhave them. Undecided.\n\nRegarding the introduction of the functions to this list,\nI would imagine people would find the commit that introduced\na function to the ban list to look for a reason why.\nCan we include a link[1] to explain why we discourage\nstrcpy and sprintf in this commit?\n\n\n[1] https://www.thegeekstuff.com/2013/06/buffer-overflow/?utm_source=feedly\n This is the best I found. So not sure if it worth it.\n\nThanks,\nStefan\n"},{"id":"353085","messageId":"20180719212700.GA13151@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CAPig+cSErDoZ5XFjfJsqfEKZ6PSKfOos=9HW-7FDXU1XEtuktA@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-19T21:27:00Z","receivedAt":"2018-07-19T21:27:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 05:11:15PM -0400, Eric Sunshine wrote:\n\n> On Thu, Jul 19, 2018 at 4:39 PM Jeff King wrote:\n> > [...]\n> > Let's start by banning strcpy() and sprintf(). It's not\n> > impossible to use these correctly, but it's easy to do so\n> > incorrectly, and there's always a better option.\n> > [...]\n> > Signed-off-by: Jeff King \n> > ---\n> > diff --git a/banned.h b/banned.h\n> > @@ -0,0 +1,19 @@\n> > +/*\n> > + * This header lists functions that have been banned from our code base,\n> > + * because they're too easy to misuse (and even if used correctly,\n> > + * complicate audits). Including this header turns them into compile-time\n> > + * errors.\n> > + */\n> \n> When the above talks about \"including this header\", the implication is\n> that it must be included _after_ the system header(s) which declare\n> the banned functions. I wonder if that requirement should be stated\n> here explicitly.\n\nHmm, does it need to be? I had originally intended it to be included\nbefore, actually, though in the end I put it later.\n\nI guess it would yield declarations like strcpy_is_banned(), which would\ncause _different_ errors (probably link-time ones).\n\n> (Probably not worth a re-roll.)\n\nYeah, I doubt it matters much either way, since the inclusion is done\nautomatically in git-compat-util.h.\n\nI had also originally imagined this to be triggered via DEVELOPER=1,\nwith something like \"-include banned.h\" in CFLAGS. But I think it\nprobably is appropriate for everybody to run it, since it shouldn't\ncause any false positives or other compilation issues.\n\nThe one I brainstormed (but forgot to mention) is that it might be\npossible for a platform to have strcpy as a macro already? In which case\nwe'd need to #undef it or risk a compilation error (even if the macro\nisn't actually used).\n\n-Peff\n"},{"id":"353086","messageId":"20180719213222.GB13151@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CAGZ79ka1yHb-485xc-sAK5iVkeyWJq4Q2g-7jzEgHuVUWLBXKg@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-19T21:32:23Z","receivedAt":"2018-07-19T21:32:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 02:15:54PM -0700, Stefan Beller wrote:\n\n> > Documentation/CodingGuidelines | 3 +++\n> \n> I'd prefer to not add more text to our documentation\n> (It is already long and in case you run into this problem\n> the error message is clear enough IMHO)\n\nI'm fine with that too. I just wondered if somebody would complain in\nthe opposite way: your patch enforces this, but we never made it an\n\"official\" guideline.\n\nBut that may be overly paranoid. Once upon a time there was some rules\nlawyering around CodingGuidelines, but I think that was successfully\nshut down and hasn't reared its head for several years.\n\n> > +#define strcpy(x,y) BANNED(strcpy)\n> > +\n> > +#ifdef HAVE_VARIADIC_MACROS\n> \n> In a split second I thought you forgot sprintf that was\n> mentioned in the commit message, but then I kept on reading\n> just to find it here. I wonder if we want put this #ifdef at the\n> beginning of the file (after the guard) as then we can have\n> a uncluttered list of banned functions here. The downside is that\n> the use of strcpy would not be banned in case you have no\n> variadic macros, but we'd still catch it quickly as most people\n> have them. Undecided.\n\nRight, exactly. We should catch what we can on lesser platforms, and\neverything on modern ones. My hope is that people would not generally\nhave to touch this file. I don't think we'll be adding banned functions\noften.\n\n> Regarding the introduction of the functions to this list,\n> I would imagine people would find the commit that introduced\n> a function to the ban list to look for a reason why.\n> Can we include a link[1] to explain why we discourage\n> strcpy and sprintf in this commit?\n\nI hoped that it was mostly common knowledge, but that's probably not a\ngood assumption. I agree if there's a good reference, we should link to\nit.\n\n> [1] https://www.thegeekstuff.com/2013/06/buffer-overflow/?utm_source=feedly\n> This is the best I found. So not sure if it worth it.\n\nYeah, this one is so-so, because it goes into a lot more detail. I think\nwe can assume that people know that overflowing a buffer is bad. Maybe\njust a short paragraph like:\n\n We'll include strcpy() and sprintf() in the initial list of banned\n functions. While these _can_ be used carefully by surrounding them\n with extra code, there's no inherent check that the size of the\n destination buffer is big enough, it's very easy to overflow the\n buffer.\n\n-Peff\n"},{"id":"353087","messageId":"20180719213300.GC13151@sigill.intra.peff.net","threadId":"48922","inReplyTo":"87y3e7aq0e.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] banned.h: mark strncpy as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-19T21:33:00Z","receivedAt":"2018-07-19T21:33:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 11:12:49PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Thu, Jul 19 2018, Jeff King wrote:\n> \n> > Since this use of strncpy was verified by manual inspection\n> > and since it doesn't trigger the automated ban-list, we're\n> > better off leaving it to keep our divergence from glibc\n> > minimal.\n> \n> FWIW it's s/glibc/gawk/. It's originally from glibc, but gawk\n> perma-forked it long ago, and an ancient copy of thath is the one we\n> use.\n\nThanks, I didn't know that. Not materially different, but it's worth\ncorrecting the commit message.\n\n-Peff\n"},{"id":"353090","messageId":"CAGZ79kaqgF5zNC0X5+EnuPhYiaav9JiSsgeXF=deryS3sKYq2A@mail.gmail.com","threadId":"48922","inReplyTo":"20180719213222.GB13151@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-19T21:47:35Z","receivedAt":"2018-07-19T21:47:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 19, 2018 at 2:32 PM Jeff King wrote:\n>\n> On Thu, Jul 19, 2018 at 02:15:54PM -0700, Stefan Beller wrote:\n>\n> > > Documentation/CodingGuidelines | 3 +++\n> >\n> > I'd prefer to not add more text to our documentation\n> > (It is already long and in case you run into this problem\n> > the error message is clear enough IMHO)\n>\n> I'm fine with that too. I just wondered if somebody would complain in\n> the opposite way: your patch enforces this, but we never made it an\n> \"official\" guideline.\n>\n> But that may be overly paranoid. Once upon a time there was some rules\n> lawyering around CodingGuidelines, but I think that was successfully\n> shut down and hasn't reared its head for several years.\n\nHeh; My preference would be to keep docs as short and concise as\npossible (and lawyering sounds like \"verbosity is a feature\" :-P) but\nstill giving advice on controversial (i.e. not enforced by a machine in\na deterministic fashion) things such as having braces around one\nstatement for example or leaving them out.\n\n> > Regarding the introduction of the functions to this list,\n> > I would imagine people would find the commit that introduced\n> > a function to the ban list to look for a reason why.\n> > Can we include a link[1] to explain why we discourage\n> > strcpy and sprintf in this commit?\n>\n> I hoped that it was mostly common knowledge, but that's probably not a\n> good assumption. I agree if there's a good reference, we should link to\n> it.\n\nIn this case I am just an advocate for a better history. Countless\ntimes I wanted\nto know *why* something is the way it is and mailing list and history are not\nvery good at that, as my specific question was not addressed there.\n\nEither it was obvious to all people involved at the time or not written down\nwhy that solution (which -- in hindsight -- sucks), was superior to the other\nsolution (which may or may not have been known at the time).\n\nSo maybe I would have rather asked, why we start out with these two\nfunctions. And you seem to call them \"obviously bad\", and you take both\nof them because they need to be handled differently due to the variadic macros.\n(And those two are \"obviously worse\" than strcat, as they are used more often.\nBut strcat, being on MS ban list[1], would do just as fine)\n\n(I agree that) Any other function can be added on top with its own\ncommit message on *why* that function. Over time I would expect that the\nreason is that we got bitten by it, or some other project got famously bitten\nby it or that some smart people came up with a list[1]. The exception is\nthis one, which basically says: \"Here is the mechanism how to do it and\n$X is the obvious thing to put in first\", which I agree with the mechanism\nas well as that $X seems bad.\n\n[1] https://msdn.microsoft.com/en-us/library/bb288454.aspx\n\nNow that I am deep down the rathole of discussing a tangent, I just found\n[2], when searching for \"how much common knowledge is wrong\", do you\nknow if there is such a list for (CS) security related things?\n\n[2] http://www.marcandangel.com/2008/06/12/60-popular-pieces-of-false-knowledge/\n\n>\n> > [1] https://www.thegeekstuff.com/2013/06/buffer-overflow/?utm_source=feedly\n> > This is the best I found. So not sure if it worth it.\n>\n> Yeah, this one is so-so, because it goes into a lot more detail. I think\n> we can assume that people know that overflowing a buffer is bad. Maybe\n> just a short paragraph like:\n>\n> We'll include strcpy() and sprintf() in the initial list of banned\n> functions. While these _can_ be used carefully by surrounding them\n> with extra code, there's no inherent check that the size of the\n> destination buffer is big enough, it's very easy to overflow the\n> buffer.\n\nSounds good to me, maybe even add \"We've had problems with that\nin the past, see 'git log -S strcpy'\", but that may be too much again.\n\nThanks,\nStefan\n"},{"id":"353092","messageId":"CAPig+cTbvKZ6Ui_ib2g2Xqbv8zFwJp9idocbWaZLxj0Lpe+YBA@mail.gmail.com","threadId":"48922","inReplyTo":"20180719212700.GA13151@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-19T21:59:47Z","receivedAt":"2018-07-19T22:00:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 19, 2018 at 5:27 PM Jeff King wrote:\n> On Thu, Jul 19, 2018 at 05:11:15PM -0400, Eric Sunshine wrote:\n> > On Thu, Jul 19, 2018 at 4:39 PM Jeff King wrote:\n> > > + * This header lists functions that have been banned from our code base,\n> > > + * because they're too easy to misuse (and even if used correctly,\n> > > + * complicate audits). Including this header turns them into compile-time\n> > > + * errors.\n> >\n> > When the above talks about \"including this header\", the implication is\n> > that it must be included _after_ the system header(s) which declare\n> > the banned functions. I wonder if that requirement should be stated\n> > here explicitly.\n>\n> Hmm, does it need to be? I had originally intended it to be included\n> before, actually, though in the end I put it later.\n> I guess it would yield declarations like strcpy_is_banned(), which would\n> cause _different_ errors (probably link-time ones).\n\nYes, that's what I meant. You'd only get link-time errors if banned.h\nwas included before the system headers (assuming I'm thinking about\nthis correctly).\n\n> > (Probably not worth a re-roll.)\n>\n> Yeah, I doubt it matters much either way, since the inclusion is done\n> automatically in git-compat-util.h.\n\nExactly.\n\n> I had also originally imagined this to be triggered via DEVELOPER=1,\n> with something like \"-include banned.h\" in CFLAGS. But I think it\n> probably is appropriate for everybody to run it, since it shouldn't\n> cause any false positives or other compilation issues.\n\nAgreed.\n\n> The one I brainstormed (but forgot to mention) is that it might be\n> possible for a platform to have strcpy as a macro already? In which case\n> we'd need to #undef it or risk a compilation error (even if the macro\n> isn't actually used).\n\nI have some recollection (perhaps long outdated or just wrong) of\nMicrosoft headers spewing deprecation warnings about \"unsafe\"\nfunctions. I don't know whether they did that by covering functions\nwith macros or by decorating the function with a deprecation attribute\nor by some other mechanism, but such concern seems well-founded.\n#undef'ing them might indeed be a very good preventative tactic.\n"},{"id":"353101","messageId":"xmqqmuumdetr.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180719203901.GA8079@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-19T22:46:08Z","receivedAt":"2018-07-19T22:46:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For enforcement, we can rely on the compiler and just\n> introduce code which breaks compilation when it sees these\n> functions. This has a few advantages:\n>\n> 1. We know it's run as part of a build cycle, so it's\n> hard to ignore. Whereas an external linter is an extra\n> step the developer needs to remember to do.\n>\n> 2. Likewise, it's basically free since the compiler is\n> parsing the code anyway.\n>\n> 3. We know it's robust against false positives (unlike a\n> grep-based linter).\n>\n> The one big disadvantage is that it will only check code\n> that is actually compiled, so it may miss code that isn't\n> triggered on your particular system. But since presumably\n> people don't add new code without compiling it (and if they\n> do, the banned function list is the least of their worries),\n> we really only care about failing to clean up old code when\n> adding new functions to the list. And that's easy enough to\n> address with a manual audit when adding a new function\n> (which is what I did for the functions here).\n>\n> That leaves only the question of how to trigger the\n> compilation error. The goals are:\n\nI actually have another question, though.\n\nIs it a downside that it is cumbersome to override? This is not a\nrhetorical question. I am not convinced there will not be a legit\ncircumstance that calling strcpy (or whatever we are going to ban)\nis the best solution and it is safe. By \"best\", what I mean is \"you\ncould instead use memcpy/strncpy/whatever\" can legitimately be\nargued with \"but still using memcpy/strncpy/whatever is inferior\nthan using strcpy in this case for such and such reasons\".\n\n"},{"id":"353103","messageId":"005801d41fbb$ed885fb0$c8991f10$@nexbridge.com","threadId":"48922","inReplyTo":"xmqqmuumdetr.fsf@gitster-ct.c.googlers.com","subject":"RE: [PATCH 1/2] introduce \"banned function\" list","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-07-19T23:55:04Z","receivedAt":"2018-07-19T23:55:27Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On July 19, 2018 6:46 PM, Junio wrote:\n> Jeff King writes:\n> \n> > For enforcement, we can rely on the compiler and just introduce code\n> > which breaks compilation when it sees these functions. This has a few\n> > advantages:\n> >\n> > 1. We know it's run as part of a build cycle, so it's\n> > hard to ignore. Whereas an external linter is an extra\n> > step the developer needs to remember to do.\n> >\n> > 2. Likewise, it's basically free since the compiler is\n> > parsing the code anyway.\n> >\n> > 3. We know it's robust against false positives (unlike a\n> > grep-based linter).\n> >\n> > The one big disadvantage is that it will only check code that is\n> > actually compiled, so it may miss code that isn't triggered on your\n> > particular system. But since presumably people don't add new code\n> > without compiling it (and if they do, the banned function list is the\n> > least of their worries), we really only care about failing to clean up\n> > old code when adding new functions to the list. And that's easy enough\n> > to address with a manual audit when adding a new function (which is\n> > what I did for the functions here).\n> >\n> > That leaves only the question of how to trigger the compilation error.\n> > The goals are:\n> \n> I actually have another question, though.\n> \n> Is it a downside that it is cumbersome to override? This is not a\nrhetorical\n> question. I am not convinced there will not be a legit circumstance that\n> calling strcpy (or whatever we are going to ban) is the best solution and\nit is\n> safe. By \"best\", what I mean is \"you could instead use\n> memcpy/strncpy/whatever\" can legitimately be argued with \"but still using\n> memcpy/strncpy/whatever is inferior than using strcpy in this case for\nsuch\n> and such reasons\".\n\nPutting on my old-guy compiler hat, this sounds like a more complex activity\nthat something akin to lint might be useful at handling. Having a\npost-processor that searches for offending functions but also supports\nannotations explaining exceptions (why you really had to use strncpy because\nthe NULL was hiding in a bad place and you promise to fix it), might be\nuseful. Personally, I'd rather know that that code compiles first and then\nviolates rules that I can fix following basic prototyping than getting\nyelled at up front - but that's just me. I can't suggest a good thing to\nhandle this, short of augmenting lint, and if we were in java, annotations\nwould be the way to go, but this seems like a problem that other products\nhave solved.\n\nCheers,\nRandall\n\n-- Brief whoami:\n NonStop developer since approximately NonStop(211288444200000000)\n UNIX developer since approximately 421664400\n-- In my real life, I talk too much.\n\n\n\n"},{"id":"353104","messageId":"20180720005404.GA2179@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CAGZ79kaqgF5zNC0X5+EnuPhYiaav9JiSsgeXF=deryS3sKYq2A@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T00:54:04Z","receivedAt":"2018-07-20T00:54:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 02:47:35PM -0700, Stefan Beller wrote:\n\n> > But that may be overly paranoid. Once upon a time there was some rules\n> > lawyering around CodingGuidelines, but I think that was successfully\n> > shut down and hasn't reared its head for several years.\n> \n> Heh; My preference would be to keep docs as short and concise as\n> possible (and lawyering sounds like \"verbosity is a feature\" :-P) but\n> still giving advice on controversial (i.e. not enforced by a machine in\n> a deterministic fashion) things such as having braces around one\n> statement for example or leaving them out.\n\nI think we literally had somebody say \"I don't have to abide by this in\na review because it wasn't in CodingGuidelines.\" But then, that is\nperhaps indicative of other problems.\n\n> So maybe I would have rather asked, why we start out with these two\n> functions. And you seem to call them \"obviously bad\", and you take both\n> of them because they need to be handled differently due to the variadic macros.\n> (And those two are \"obviously worse\" than strcat, as they are used more often.\n> But strcat, being on MS ban list[1], would do just as fine)\n\nOoh, strcat is another one that should be added.\n\nI actually thought about splitting it into three commits (introduce\nmechanism, then one per function), but it felt like stringing it out.\nYou are probably right, though, that each function deserves its own\nexplanation. And the commit message is already quite long.\n\n> > We'll include strcpy() and sprintf() in the initial list of banned\n> > functions. While these _can_ be used carefully by surrounding them\n> > with extra code, there's no inherent check that the size of the\n> > destination buffer is big enough, it's very easy to overflow the\n> > buffer.\n> \n> Sounds good to me, maybe even add \"We've had problems with that\n> in the past, see 'git log -S strcpy'\", but that may be too much again.\n\nActually, that's a good point. We've had actual bugs due strcpy(). I can\nsimilarly point to bad uses of strcat().\n\nI think I have sufficient fodder for a re-roll along these lines\n(assuming we like the idea at all; Junio seemed to have some\nreservations, but I'll reply there separately).\n\n-Peff\n"},{"id":"353105","messageId":"20180720005545.GB2179@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CAPig+cTbvKZ6Ui_ib2g2Xqbv8zFwJp9idocbWaZLxj0Lpe+YBA@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T00:55:45Z","receivedAt":"2018-07-20T00:55:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 05:59:47PM -0400, Eric Sunshine wrote:\n\n> > The one I brainstormed (but forgot to mention) is that it might be\n> > possible for a platform to have strcpy as a macro already? In which case\n> > we'd need to #undef it or risk a compilation error (even if the macro\n> > isn't actually used).\n> \n> I have some recollection (perhaps long outdated or just wrong) of\n> Microsoft headers spewing deprecation warnings about \"unsafe\"\n> functions. I don't know whether they did that by covering functions\n> with macros or by decorating the function with a deprecation attribute\n> or by some other mechanism, but such concern seems well-founded.\n> #undef'ing them might indeed be a very good preventative tactic.\n\nYeah, these functions are definitely on their \"SDL banned list\". I don't\nknow how they implement that. At that level, I'd really expect it to be\ndone with a deprecated attribute next to the declaration (I also\nconsidered trying to add deprecation attributes, too, but I think it's\nhard to do without re-declaring the function, and anyway it's \"just\" a\nwarning).\n\n-Peff\n"},{"id":"353106","messageId":"20180720010808.GC2179@sigill.intra.peff.net","threadId":"48922","inReplyTo":"xmqqmuumdetr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T01:08:08Z","receivedAt":"2018-07-20T01:08:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 03:46:08PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > For enforcement, we can rely on the compiler and just\n> > introduce code which breaks compilation when it sees these\n> > functions. This has a few advantages:\n> >\n> > 1. We know it's run as part of a build cycle, so it's\n> > hard to ignore. Whereas an external linter is an extra\n> > step the developer needs to remember to do.\n> >\n> > 2. Likewise, it's basically free since the compiler is\n> > parsing the code anyway.\n> >\n> > 3. We know it's robust against false positives (unlike a\n> > grep-based linter).\n> >\n> > The one big disadvantage is that it will only check code\n> > that is actually compiled, so it may miss code that isn't\n> > triggered on your particular system. But since presumably\n> > people don't add new code without compiling it (and if they\n> > do, the banned function list is the least of their worries),\n> > we really only care about failing to clean up old code when\n> > adding new functions to the list. And that's easy enough to\n> > address with a manual audit when adding a new function\n> > (which is what I did for the functions here).\n> >\n> > That leaves only the question of how to trigger the\n> > compilation error. The goals are:\n> \n> I actually have another question, though.\n> \n> Is it a downside that it is cumbersome to override? This is not a\n> rhetorical question. I am not convinced there will not be a legit\n> circumstance that calling strcpy (or whatever we are going to ban)\n> is the best solution and it is safe. By \"best\", what I mean is \"you\n> could instead use memcpy/strncpy/whatever\" can legitimately be\n> argued with \"but still using memcpy/strncpy/whatever is inferior\n> than using strcpy in this case for such and such reasons\".\n\nIn my opinion, no, this is not a problem.\n\nMy plan is to only add functions which are truly worthless. So with\nstrcpy(), for example, you _can_ make sure the buffer is big enough\nbefore calling strcpy. But to do so, you by definition must have just\ncalled strlen(), at which point you may as well use memcpy(). It's more\nobviously correct, and it's more efficient.\n\nDitto for sprintf, where you should _always_ be using at least xsnprintf\n(or some better tool, depending on the situation). And for strncpy,\nstrlcpy (or again, some better tool) is strictly an improvement. If\nthese were our functions, I would literally delete them from our\ncodebase. This is the moral equivalent. ;)\n\nContrast this with memcpy(). This is on Microsoft's SDL banned list[1],\nbut I think it's silly for it to be. I would never add it to this list.\n\nLikewise snprintf(). That is a function that _can_ be dangerous due to\nunexpected truncation, and I think we should avoid it in general. But I\nwould not ban it, as it is the correct tool in a few situations (you\nhave a fixed buffer and returning a truncation error code is fine --\nmany of our syscall wrappers are in this exact situation). So I would\nprobably not add it to the ban list (but I feel less strongly than I do\nabout memcpy).\n\nThe nuclear option for overriding is \"#undef that-function\" in a\nparticular instance. That covers the rest of the translation unit, but I\nthink that's probably fine. If we have a function which is mostly\nquestionable, but we need it sometimes, then we ought to be putting a\nsane wrapper around it. And _that_ wrapper can sit in its own file and\n#undef as it pleases, keeping the unsafety contained within.\n\nDon't get me wrong, if you have another suggestion that allows easier\none-off overrides, I'm happy to hear it. But I think building this into\nthe compilation step and having it on-by-default is a huge advantage in\nsimplicity and having people remember to run it. And I couldn't come up\nwith a better way to do that.\n\n-Peff\n"},{"id":"353107","messageId":"20180720011251.GA2667@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180720010808.GC2179@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T01:12:52Z","receivedAt":"2018-07-20T01:12:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 09:08:08PM -0400, Jeff King wrote:\n\n> Contrast this with memcpy(). This is on Microsoft's SDL banned list[1],\n> but I think it's silly for it to be. I would never add it to this list.\n\nI forgot my footnote, which was going to be:\n\n I'm bringing up that list not because I think it's necessarily a good\n list, but because it's _a_ list. And as I was recently subjected to an\n audit that referenced it, I've been thinking a lot about ban-lists and\n whether they are useful (and specifically for which functions).\n\n It's at https://msdn.microsoft.com/en-us/library/bb288454.aspx if\n you're curious, but again, that is absolutely not the ban-list I am\n working towards. To what I posted already, I'd probably add strcat()\n and vsprintf() based on discussions here, and then call it done.\n\n-Peff\n"},{"id":"353119","messageId":"xmqqa7qmckwi.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180720010808.GC2179@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T09:32:29Z","receivedAt":"2018-07-20T09:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Is it a downside that it is cumbersome to override? This is not a\n>> rhetorical question. I am not convinced there will not be a legit\n>> circumstance that calling strcpy (or whatever we are going to ban)\n>> is the best solution and it is safe. By \"best\", what I mean is \"you\n>> could instead use memcpy/strncpy/whatever\" can legitimately be\n>> argued with \"but still using memcpy/strncpy/whatever is inferior\n>> than using strcpy in this case for such and such reasons\".\n>\n> In my opinion, no, this is not a problem.\n>\n> My plan is to only add functions which are truly worthless.\n\nOK.\n\n> Contrast this with memcpy(). This is on Microsoft's SDL banned list[1],\n> but I think it's silly for it to be. I would never add it to this list.\n\nA tangent, but is that because they want you to use memmove()\ninstead so that you do not have to worry about overlapping copies,\nperhaps?\n\n"},{"id":"353120","messageId":"153e7256-cc45-c33e-d64a-27319b42a489@gmail.com","threadId":"48922","inReplyTo":"20180719203901.GA8079@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-20T12:42:56Z","receivedAt":"2018-07-20T12:49:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/19/2018 4:39 PM, Jeff King wrote:\n> There are a few standard C functions (like strcpy) which are\n> easy to misuse. We generally discourage these in reviews,\n> but we haven't put advice in CodingGuidelines, nor provided\n> any automated enforcement. The latter is especially\n> important because it's more consistent, and it can often\n> save a round-trip of review.\n>\n> Let's start by banning strcpy() and sprintf(). It's not\n> impossible to use these correctly, but it's easy to do so\n> incorrectly, and there's always a better option.\n>\n> For enforcement, we can rely on the compiler and just\n> introduce code which breaks compilation when it sees these\n> functions. This has a few advantages:\n>\n> 1. We know it's run as part of a build cycle, so it's\n> hard to ignore. Whereas an external linter is an extra\n> step the developer needs to remember to do.\n>\n> 2. Likewise, it's basically free since the compiler is\n> parsing the code anyway.\n>\n> 3. We know it's robust against false positives (unlike a\n> grep-based linter).\n\nI know I'm late to the party, but I just wanted to chime in that I think \nthis is a great idea. The more checks we have as part of the developer \ncycle means we have fewer checks that need to be caught manually in \nreview (or can be missed in review).\n\nThanks,\n\n-Stolee\n\n"},{"id":"353122","messageId":"20180720132241.GN30706@thunk.org","threadId":"48922","inReplyTo":"20180720010808.GC2179@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Theodore Y. Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2018-07-20T13:22:41Z","receivedAt":"2018-07-20T13:22:55Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Thu, Jul 19, 2018 at 09:08:08PM -0400, Jeff King wrote:\n> Ditto for sprintf, where you should _always_ be using at least xsnprintf\n> (or some better tool, depending on the situation). And for strncpy,\n> strlcpy (or again, some better tool) is strictly an improvement.\n\nNitpick: this may be true for git, but it's not strictly true in all\ncases. I actually have a (non-git) case where strncpy *is* the right\ntool. And this is one where I'm copying a NUL-terminated string into\na fixed-length charater array (in the ext4 superblock) which is *not*\nNUL-terminated. In that case, strncpy works(); but strlcpy() does not\ndo what I want.\n\nSo I used strncpy() advisedly, and I ignore people running Coccinelle\nscripts and blindly sending patches to \"fix\" ext4.\n\nBut perhaps that's also a solution for git? You don't have to\nnecessarily put them on a banned list; you could instead have some\nhandy, pre-set scripts that scan the entire code base looking for\n\"bad\" functions with cleanups automatically suggested. This could be\nrun at patch review time, and/or periodically (especially before a\nrelease).\n\n\t\t\t\t\t\t- Ted\n"},{"id":"353125","messageId":"CACsJy8BxXyY6h8iN-RExBHASB6FkFLKDkrE_h+gH0A1k2tVd6w@mail.gmail.com","threadId":"48922","inReplyTo":"20180719203901.GA8079@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-20T14:41:34Z","receivedAt":"2018-07-20T14:42:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jul 19, 2018 at 10:40 PM Jeff King wrote:\n>\n> There are a few standard C functions (like strcpy) which are\n> easy to misuse. We generally discourage these in reviews,\n> but we haven't put advice in CodingGuidelines, nor provided\n> any automated enforcement. The latter is especially\n> important because it's more consistent, and it can often\n> save a round-trip of review.\n>\n> Let's start by banning strcpy() and sprintf(). It's not\n> impossible to use these correctly, but it's easy to do so\n> incorrectly, and there's always a better option.\n\nIs it possible to extend this to ban variables as well? I'm still\ngoing to delete the_index from library code. Once that work is done I\nwill ban it from entering again (it's only allowed in builtin/ for\nexample). The next target after that would be the_repository.\n\nRight now I will do this by not declaring the variable [2], which ends\nup with a much less friendly compile warning. I did something similar\n[1] in an earlier iteration but did not do extensive research on this\ntopic like you did.\n\n[1] https://public-inbox.org/git/20180606073933.14755-1-pclouds@gmail.com/T/\n[2] https://public-inbox.org/git/20180616054157.32433-16-pclouds@gmail.com/\n-- \nDuy\n"},{"id":"353159","messageId":"20180720174524.GB22486@sigill.intra.peff.net","threadId":"48922","inReplyTo":"xmqqa7qmckwi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T17:45:24Z","receivedAt":"2018-07-20T17:45:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 20, 2018 at 02:32:29AM -0700, Junio C Hamano wrote:\n\n> > Contrast this with memcpy(). This is on Microsoft's SDL banned list[1],\n> > but I think it's silly for it to be. I would never add it to this list.\n> \n> A tangent, but is that because they want you to use memmove()\n> instead so that you do not have to worry about overlapping copies,\n> perhaps?\n\nThat was my first thought, too, but nope. They recommend memcpy_s()\ninstead. Which in my opinion adds very little value, while missing the\nmost common misuse of memcpy I've seen in practice (the overlapping\nthing).\n\nHelpers like our COPY_ARRAY() are much more useful for preventing sizing\nmistakes, IMHO. But again, I'd never ban memcpy. The right tool for\nencouraging COPY_ARRAY() is coccinelle (because the matching is\ncomplicated, but also because we can mechanically turn it into the right\nthing, whereas a strcpy is going to require some manual reworking).\n\n-Peff\n"},{"id":"353161","messageId":"CABPp-BGt2EEW4aFt8TYyy1fATPRW2rnNxpMuGcS0YR_gqs3=ig@mail.gmail.com","threadId":"48922","inReplyTo":"CACsJy8BxXyY6h8iN-RExBHASB6FkFLKDkrE_h+gH0A1k2tVd6w@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-20T17:48:37Z","receivedAt":"2018-07-20T17:48:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 20, 2018 at 7:41 AM, Duy Nguyen wrote:\n> On Thu, Jul 19, 2018 at 10:40 PM Jeff King wrote:\n>>\n>> There are a few standard C functions (like strcpy) which are\n>> easy to misuse. We generally discourage these in reviews,\n>> but we haven't put advice in CodingGuidelines, nor provided\n>> any automated enforcement. The latter is especially\n>> important because it's more consistent, and it can often\n>> save a round-trip of review.\n>>\n>> Let's start by banning strcpy() and sprintf(). It's not\n>> impossible to use these correctly, but it's easy to do so\n>> incorrectly, and there's always a better option.\n>\n> Is it possible to extend this to ban variables as well? I'm still\n> going to delete the_index from library code. Once that work is done I\n\nOr perhaps constants, such as PATH_MAX to avoid problems like this one\nfrom 2.18.0 timeframe:\nhttps://public-inbox.org/git/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/\n"},{"id":"353162","messageId":"20180720175602.GC22486@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180720132241.GN30706@thunk.org","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T17:56:03Z","receivedAt":"2018-07-20T17:56:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 20, 2018 at 09:22:41AM -0400, Theodore Y. Ts'o wrote:\n\n> On Thu, Jul 19, 2018 at 09:08:08PM -0400, Jeff King wrote:\n> > Ditto for sprintf, where you should _always_ be using at least xsnprintf\n> > (or some better tool, depending on the situation). And for strncpy,\n> > strlcpy (or again, some better tool) is strictly an improvement.\n> \n> Nitpick: this may be true for git, but it's not strictly true in all\n> cases. I actually have a (non-git) case where strncpy *is* the right\n> tool. And this is one where I'm copying a NUL-terminated string into\n> a fixed-length charater array (in the ext4 superblock) which is *not*\n> NUL-terminated. In that case, strncpy works(); but strlcpy() does not\n> do what I want.\n\nThanks for an interesting (and exotic) counter-point. For strcpy(), you\nalways have to run strlen() anyway to know you're not going to overflow,\nso you might as well memcpy() at that point. But if you really are OK\nwith truncation, then using strncpy() lets you copy while walking over\nthe string only once, which is more efficient. On the other hand,\nstrncpy() fills out NULs to the end of the destination array, so you are\npaying an extra cost for small strings.\n\n> So I used strncpy() advisedly, and I ignore people running Coccinelle\n> scripts and blindly sending patches to \"fix\" ext4.\n\nEven after hearing your counter-point, I think I'm still OK with the\nban. Because as you've noticed, even if the call is fine, it carries an\nongoing maintenance cost. Every time you (or somebody else) audits, you\nhave to skip over that known-good call. And you have to deal with\nwell-meaning patches. So to me there's value in eliminating it even if\nit's an acceptable tool.\n\nWe don't have any remaining calls to strncpy() in Git, so this lets us\npunt on deciding if the ban is worth changing what's there. We can wait\nfor somebody to decide they _really_ need strncpy, and we can discuss it\nthen with a concrete case.\n\n> But perhaps that's also a solution for git? You don't have to\n> necessarily put them on a banned list; you could instead have some\n> handy, pre-set scripts that scan the entire code base looking for\n> \"bad\" functions with cleanups automatically suggested. This could be\n> run at patch review time, and/or periodically (especially before a\n> release).\n\nWe do this already with coccinelle for some transformations. But I think\nthere's real value in linting that's always run, and is cheap. Catching\nthese things early means we don't have to spend time on them in review,\nor dealing with follow-up patches.\n\n-Peff\n"},{"id":"353163","messageId":"20180720180039.GD22486@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CACsJy8BxXyY6h8iN-RExBHASB6FkFLKDkrE_h+gH0A1k2tVd6w@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T18:00:39Z","receivedAt":"2018-07-20T18:00:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 20, 2018 at 04:41:34PM +0200, Duy Nguyen wrote:\n\n> > Let's start by banning strcpy() and sprintf(). It's not\n> > impossible to use these correctly, but it's easy to do so\n> > incorrectly, and there's always a better option.\n> \n> Is it possible to extend this to ban variables as well? I'm still\n> going to delete the_index from library code. Once that work is done I\n> will ban it from entering again (it's only allowed in builtin/ for\n> example). The next target after that would be the_repository.\n> \n> Right now I will do this by not declaring the variable [2], which ends\n> up with a much less friendly compile warning. I did something similar\n> [1] in an earlier iteration but did not do extensive research on this\n> topic like you did.\n\nIMHO just not declaring the variable is the right move (and is what I\nwould do with these functions if libc wasn't already declaring them).\n\nThe compile error may be less friendly, but these things are temporary\nas topics-in-flight catch up. Whereas strcpy() will be with us forever.\n\nI also think that removing variables is a special case of a larger\ntechnique: when we change function semantics, we try to change the\nsignatures, too. So there you'll get a type error, or not enough\narguments, or whatever. And the next step is usually \"git log\" or \"git\nblame\" to see what happened, and what the conversion should look like.\n\n-Peff\n"},{"id":"353164","messageId":"20180720180427.GE22486@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CABPp-BGt2EEW4aFt8TYyy1fATPRW2rnNxpMuGcS0YR_gqs3=ig@mail.gmail.com","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T18:04:27Z","receivedAt":"2018-07-20T18:04:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 20, 2018 at 10:48:37AM -0700, Elijah Newren wrote:\n\n> > Is it possible to extend this to ban variables as well? I'm still\n> > going to delete the_index from library code. Once that work is done I\n> \n> Or perhaps constants, such as PATH_MAX to avoid problems like this one\n> from 2.18.0 timeframe:\n> https://public-inbox.org/git/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/\n\nI've been slowly trying to eradicate PATH_MAX from our code base. And I\nwould be happy with an eventual automated ban there. Unlike the_index,\nit comes from the system, so it's in the same boat as strcpy() etc.\n\nThat said, I think it's less urgent. The urgent problem fixed by the\npatch you linked was the use of strcpy() to overflow the buffer. Without\nthat, it just becomes a normal bug where we do not handle long paths\nwell on some operating systems.\n\n-Peff\n"},{"id":"353169","messageId":"xmqqo9f1bupp.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180719203259.GA7869@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fail compilation with strcpy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T18:58:10Z","receivedAt":"2018-07-20T18:58:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is a patch series to address the discussion in the thread at:\n>\n> https://public-inbox.org/git/20180713204350.GA16999@sigill.intra.peff.net/\n>\n> Basically, the question was: can we declare strcpy banned and have a\n> linter save us the trouble of finding it in review. The answer is yes,\n> the compiler is good at that. ;)\n>\n> There are probably as many lists of banned functions as there are coding\n> style documents. I don't agree with every entry in the ones I've seen.\n> And in many cases coccinelle is a better choice, because the problem is\n> not \"this function is so bad your patch should not even make it to the\n> list with it\", but \"don't do it like A; we prefer to do it like B\n> instead\". And coccinelle does the latter more flexibly and\n> automatically.\n>\n> So I tried to pick some obvious and uncontroversial candidates here.\n> gets() could be another one, but it's mostly banned already (it's out of\n> the standard, and most libcs mark it with a deprecated attribute).\n>\n> Note that this needs to be applied on top of 022d2ac1f3 (blame: prefer\n> xsnprintf to strcpy for colors, 2018-07-13) or it will complain loudly. :)\n>\n> [1/2]: introduce \"banned function\" list\n> [2/2]: banned.h: mark strncpy as banned\n\nHmph, there is no use of any banned function in hex.c, but when\nthis topic is merged to 'pu', I seem to get this:\n\n$ make DEVELOPER=1 hex.o\nGIT_VERSION = 2.18.0.758.g18f90b35b8\n CC hex.o\nIn file included from git-compat-util.h:1250:0,\n from cache.h:4,\n from hex.c:1:\nbanned.h:14:0: error: \"strncpy\" redefined [-Werror]\n #define strncpy(x,y,n) BANNED(strncpy)\n \nIn file included from /usr/include/string.h:630:0,\n from git-compat-util.h:165,\n from cache.h:4,\n from hex.c:1:\n/usr/include/x86_64-linux-gnu/bits/string2.h:84:0: note: this is the location of the previous definition\n # define strncpy(dest, src, n) __builtin_strncpy (dest, src, n)\n \ncc1: all warnings being treated as errors\nMakefile:2279: recipe for target 'hex.o' failed\nmake: *** [hex.o] Error 1\n\n"},{"id":"353170","messageId":"xmqqk1ppbuhg.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180720175602.GC22486@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] introduce \"banned function\" list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T19:03:07Z","receivedAt":"2018-07-20T19:03:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks for an interesting (and exotic) counter-point. For strcpy(), you\n> always have to run strlen() anyway to know you're not going to overflow,\n> so you might as well memcpy() at that point. But if you really are OK\n> with truncation, then using strncpy() lets you copy while walking over\n> the string only once, which is more efficient. On the other hand,\n> strncpy() fills out NULs to the end of the destination array, so you are\n> paying an extra cost for small strings.\n>\n>> So I used strncpy() advisedly, and I ignore people running Coccinelle\n>> scripts and blindly sending patches to \"fix\" ext4.\n\nGiven that strncpy() was invented in V7 days for specifically the\npurpose of filling the filename field, it is not surprising at all\nthat it is the perfect tool for the same purpose in ext4 ;-)\n\n> We don't have any remaining calls to strncpy() in Git, so this lets us\n> punt on deciding if the ban is worth changing what's there. We can wait\n> for somebody to decide they _really_ need strncpy, and we can discuss it\n> then with a concrete case.\n\nYes, and the plan (or at least your plan) is to limit the banned\nlist to things that really has no reason to be used, the above\nsounds like a good approach.\n\n"},{"id":"353175","messageId":"20180720191839.GB26403@sigill.intra.peff.net","threadId":"48922","inReplyTo":"xmqqo9f1bupp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/2] fail compilation with strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-20T19:18:40Z","receivedAt":"2018-07-20T19:18:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 20, 2018 at 11:58:10AM -0700, Junio C Hamano wrote:\n\n> > Note that this needs to be applied on top of 022d2ac1f3 (blame: prefer\n> > xsnprintf to strcpy for colors, 2018-07-13) or it will complain loudly. :)\n> >\n> > [1/2]: introduce \"banned function\" list\n> > [2/2]: banned.h: mark strncpy as banned\n> \n> Hmph, there is no use of any banned function in hex.c, but when\n> this topic is merged to 'pu', I seem to get this:\n\nInteresting. Builds fine for me even merged to the latest push-out of\npu. But...\n\n> $ make DEVELOPER=1 hex.o\n> GIT_VERSION = 2.18.0.758.g18f90b35b8\n> CC hex.o\n> In file included from git-compat-util.h:1250:0,\n> from cache.h:4,\n> from hex.c:1:\n> banned.h:14:0: error: \"strncpy\" redefined [-Werror]\n> #define strncpy(x,y,n) BANNED(strncpy)\n> \n> In file included from /usr/include/string.h:630:0,\n> from git-compat-util.h:165,\n> from cache.h:4,\n> from hex.c:1:\n> /usr/include/x86_64-linux-gnu/bits/string2.h:84:0: note: this is the location of the previous definition\n> # define strncpy(dest, src, n) __builtin_strncpy (dest, src, n)\n\nI suspect it has more to do with system/libc differences between our\nmachines, anyway. There was discussion elsewhere in the thread about the\nneed to #undef before redefining. I guess this answers that question.\n\nI'll include that in the re-roll, and you can just ignore the v1 patches\nI sent for now.\n\n-Peff\n"},{"id":"353193","messageId":"xmqqfu0da86b.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180720191839.GB26403@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fail compilation with strcpy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T21:50:20Z","receivedAt":"2018-07-20T21:50:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I suspect it has more to do with system/libc differences between our\n> machines, anyway. There was discussion elsewhere in the thread about the\n> need to #undef before redefining. I guess this answers that question.\n\nYes, that is it. For now I can squash this in before pushing it out\non 'pu'.\n\n banned.h | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex ae6aaaa4a9..d138f3ecf2 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -10,11 +10,17 @@\n \n #define BANNED(func) sorry_##func##_is_a_banned_function()\n \n+#undef strcpy\n #define strcpy(x,y) BANNED(strcpy)\n+\n+#undef strncpy\n #define strncpy(x,y,n) BANNED(strncpy)\n \n #ifdef HAVE_VARIADIC_MACROS\n+\n+#undef sprintf\n #define sprintf(...) BANNED(sprintf)\n+\n #endif\n \n #endif /* BANNED_H */\n"},{"id":"353447","messageId":"20180724092329.GA24250@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180719203259.GA7869@sigill.intra.peff.net","subject":"[PATCH v2 0/4] fail compilation with strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T09:23:29Z","receivedAt":"2018-07-24T09:23:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 19, 2018 at 04:32:59PM -0400, Jeff King wrote:\n\n> This is a patch series to address the discussion in the thread at:\n> \n> https://public-inbox.org/git/20180713204350.GA16999@sigill.intra.peff.net/\n> \n> Basically, the question was: can we declare strcpy banned and have a\n> linter save us the trouble of finding it in review. The answer is yes,\n> the compiler is good at that. ;)\n\nHere's a v2 that I think addresses the comments on the earlier series.\nThanks everybody for your review.\n\nChanges here include:\n\n - drop the mention in CodingGuidelines. That list is already long, and\n we don't need to to waste mental effort on things that will be\n enforced automatically\n\n - we now #undef all macros before defining them to avoid redefinition\n warnings\n\n - the first patch now covers _just_ strcpy(), and each function gets\n its own patch with an explanation (and suggested alternatives). My\n thought is that these should be easy to dig up via blame, \"log -S\",\n or \"log --grep\". Though another option would be comments in banned.h.\n\n - added strcat and vsprintf to the banned list\n\n - tweaked the first patch's commit message for clarity and to address\n points raised in discussion\n\nAs before, this needs to go on top of 022d2ac1f3 (which I hope should\nhit master soon anyway).\n\n [1/4]: automatically ban strcpy()\n [2/4]: banned.h: mark strcat() as banned\n [3/4]: banned.h: mark sprintf() as banned\n [4/4]: banned.h: mark strncpy() as banned\n\n banned.h | 30 ++++++++++++++++++++++++++++++\n git-compat-util.h | 6 ++++++\n 2 files changed, 36 insertions(+)\n create mode 100644 banned.h\n\n-Peff\n"},{"id":"353448","messageId":"20180724092618.GA3288@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180724092329.GA24250@sigill.intra.peff.net","subject":"[PATCH v2 1/4] automatically ban strcpy()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T09:26:18Z","receivedAt":"2018-07-24T09:26:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a few standard C functions (like strcpy) which are\neasy to misuse. E.g.:\n\n char path[PATH_MAX];\n strcpy(path, arg);\n\nmay overflow the \"path\" buffer. Sometimes there's an earlier\nconstraint on the size of \"arg\", but even in such a case\nit's hard to verify that the code is correct. If the size\nreally is unbounded, you're better off using a dynamic\nhelper like strbuf:\n\n struct strbuf path = STRBUF_INIT;\n strbuf_addstr(path, arg);\n\nor if it really is bounded, then use xsnprintf to show your\nexpectation (and get a run-time assertion):\n\n char path[PATH_MAX];\n xsnprintf(path, sizeof(path), \"%s\", arg);\n\nwhich makes further auditing easier.\n\nWe'd usually catch undesirable code like this in a review,\nbut there's no automated enforcement. Adding that\nenforcement can help us be more consistent and save effort\n(and a round-trip) during review.\n\nThis patch teaches the compiler to report an error when it\nsees strcpy (and will become a model for banning a few other\nfunctions). This has a few advantages over a separate\nlinting tool:\n\n 1. We know it's run as part of a build cycle, so it's\n hard to ignore. Whereas an external linter is an extra\n step the developer needs to remember to do.\n\n 2. Likewise, it's basically free since the compiler is\n parsing the code anyway.\n\n 3. We know it's robust against false positives (unlike a\n grep-based linter).\n\nThe two big disadvantages are:\n\n 1. We'll only check code that is actually compiled, so it\n may miss code that isn't triggered on your particular\n system. But since presumably people don't add new code\n without compiling it (and if they do, the banned\n function list is the least of their worries), we really\n only care about failing to clean up old code when\n adding new functions to the list. And that's easy\n enough to address with a manual audit when adding a new\n function.\n\n 2. If this ends up generating false positives, it's going\n to be harder to disable (as opposed to a separate\n linter, which may have mechanisms for overriding a\n particular case).\n\n But the intent is to only ban functions which are\n obviously bad, and for which we accept using an\n alternative even when this particular use isn't buggy\n (e.g., the xsnprintf alternative above).\n\nThe implementation here is simple: we'll define a macro for\nthe banned function which replaces it with a descriptively\nnamed but undefined function. Replacing it with any invalid\ncode would work (since we just want to break compilation).\nBut ideally we'd meet these goals:\n\n - it should be portable; ideally this would trigger\n everywhere, and does not need to be part of a DEVELOPER=1\n setup (because unlike warnings which may depend on the\n compiler or system, this is a clear indicator of\n something wrong in the code).\n\n - it should generate a readable error that gives the\n developer a clue what happened\n\n - it should avoid generating too much other cruft that\n makes it hard to see the actual error\n\n - it should mention the original callsite in the error\n\nThe output with this patch looks like this (using gcc 7, on\na checkout without 022d2ac1f3, which removes the final\nstrcpy from blame.c):\n\n CC builtin/blame.o\n In file included from ./git-compat-util.h:1242:0,\n from ./cache.h:4,\n from builtin/blame.c:8:\n builtin/blame.c: In function ‘cmd_blame’:\n ./banned.h:11:22: error: implicit declaration of function ‘sorry_strcpy_is_a_banned_function’ [-Werror=implicit-function-declaration]\n #define BANNED(func) sorry_##func##_is_a_banned_function()\n ^\n ./banned.h:13:21: note: in expansion of macro ‘BANNED’\n #define strcpy(x,y) BANNED(strcpy)\n ^~~~~~\n builtin/blame.c:1074:4: note: in expansion of macro ‘strcpy’\n strcpy(repeated_meta_color, GIT_COLOR_CYAN);\n ^~~~~~\n\nThis pretty clearly shows the original callsite along with\nthe descriptive function name, and it mentions banned.h,\nwhich contains more information.\n\nWhat's not perfectly ideal is:\n\n 1. We'll only trigger with -Wimplicit-function-declaration\n (and only stop compilation with -Werror). These are\n generally enabled by DEVELOPER=1. If you _don't_ have\n that set, we'll still catch the problem, but only at\n link-time, with a slightly less useful message:\n\n /usr/bin/ld: builtin/blame.o: in function `cmd_blame':\n /home/peff/tmp/builtin/blame.c:1074: undefined reference to `sorry_strcpy_is_a_banned_function'\n collect2: error: ld returned 1 exit status\n\n If instead we convert this to a reference to an\n undefined variable, that always dies immediately. But\n gcc seems to print the set of errors twice, which\n clutters things up.\n\n 2. The expansion through BANNED() adds an extra layer of\n error. Curiously, though, removing it (and just\n expanding strcpy directly to the bogus function name)\n causes gcc _not_ to report the original line of code.\n\nSo experimentally, this seems to behave pretty well on gcc.\nClang looks OK, too, though:\n\n - it actually produces a better message for the undeclared\n identifier than for the implicit function (it doesn't\n double the errors, and for the implicit function it\n actually produces an extra complaint about\n strict-prototypes).\n\n - the BANNED indirection has no effect (it shows the\n original line of code either way)\n\nSo if we want to optimize for clang's output, we could\nswitch both of those decisions.\n\nNote that the linux kernel has a similar mechanism which\ngoes by BUILD_BUG_ON_MSG(). It works by declaring a one-off\nfunction with gcc's error attribute. That doesn't work for\nus here, because we'd like to work even on non-gcc\ncompilers (and in my opinion the compiler output is actually\n_less_ readable).\n\nSigned-off-by: Jeff King \n---\n banned.h | 16 ++++++++++++++++\n git-compat-util.h | 6 ++++++\n 2 files changed, 22 insertions(+)\n create mode 100644 banned.h\n\ndiff --git a/banned.h b/banned.h\nnew file mode 100644\nindex 0000000000..c50091ad7d\n--- /dev/null\n+++ b/banned.h\n@@ -0,0 +1,16 @@\n+#ifndef BANNED_H\n+#define BANNED_H\n+\n+/*\n+ * This header lists functions that have been banned from our code base,\n+ * because they're too easy to misuse (and even if used correctly,\n+ * complicate audits). Including this header turns them into compile-time\n+ * errors.\n+ */\n+\n+#define BANNED(func) sorry_##func##_is_a_banned_function()\n+\n+#undef strcpy\n+#define strcpy(x,y) BANNED(strcpy)\n+\n+#endif /* BANNED_H */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9a64998b24..89d37095c7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1239,4 +1239,10 @@ extern void unleak_memory(const void *ptr, size_t len);\n #define UNLEAK(var) do {} while (0)\n #endif\n \n+/*\n+ * This include must come after system headers, since it introduces macros that\n+ * replace system names.\n+ */\n+#include \"banned.h\"\n+\n #endif\n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353449","messageId":"20180724092639.GB3288@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180724092329.GA24250@sigill.intra.peff.net","subject":"[PATCH v2 2/4] banned.h: mark strcat() as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T09:26:39Z","receivedAt":"2018-07-24T09:26:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The strcat() function has all of the same overflow problems\nas strcpy(). And as a bonus, it's easy to end up\naccidentally quadratic, as each subsequent call has to walk\nthrough the existing string.\n\nThe last strcat() call went away in f063d38b80 (daemon: use\ncld->env_array when re-spawning, 2015-09-24). In general,\nstrcat() can be replaced either with a dynamic string\n(strbuf or xstrfmt), or with xsnprintf if you know the\nlength is bounded.\n\nSigned-off-by: Jeff King \n---\n banned.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex c50091ad7d..32e0bae01d 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -12,5 +12,7 @@\n \n #undef strcpy\n #define strcpy(x,y) BANNED(strcpy)\n+#undef strcat\n+#define strcat(x,y) BANNED(strcat)\n \n #endif /* BANNED_H */\n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353450","messageId":"20180724092719.GC3288@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180724092329.GA24250@sigill.intra.peff.net","subject":"[PATCH v2 3/4] banned.h: mark sprintf() as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T09:27:19Z","receivedAt":"2018-07-24T09:27:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The sprintf() function (and its variadic form vsprintf) make\nit easy to accidentally introduce a buffer overflow. If\nyou're thinking of using them, you're better off either\nusing a dynamic string (strbuf or xstrfmt), or xsnprintf if\nyou really know that you won't overflow. The last sprintf()\ncall went away quite a while ago in f0766bf94e (fsck: use\nfor_each_loose_file_in_objdir, 2015-09-24).\n\nNote that we respect HAVE_VARIADIC_MACROS here, which some\nancient platforms lack. As a fallback, we can just \"guess\"\nthat the caller will provide 3 arguments. If they do, then\nthe macro will work as usual. If not, then they'll get a\nslightly less useful error, like:\n\n git.c:718:24: error: macro \"sprintf\" passed 3 arguments, but takes just 2\n\nThat's not ideal, but it at least alerts them to the problem\narea. And anyway, we're primarily targeting people adding\nnew code. Most developers should be on modern enough\nplatforms to see the normal \"good\" error message.\n\nSigned-off-by: Jeff King \n---\n banned.h | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex 32e0bae01d..c83d6cb9df 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -15,4 +15,14 @@\n #undef strcat\n #define strcat(x,y) BANNED(strcat)\n \n+#undef sprintf\n+#undef vsprintf\n+#ifdef HAVE_VARIADIC_MACROS\n+#define sprintf(...) BANNED(sprintf)\n+#define vsprintf(...) BANNED(vsprintf)\n+#else\n+#define sprintf(buf,fmt,arg) BANNED(sprintf)\n+#define vsprintf(buf,fmt,arg) BANNED(sprintf)\n+#endif\n+\n #endif /* BANNED_H */\n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353451","messageId":"20180724092828.GD3288@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180724092329.GA24250@sigill.intra.peff.net","subject":"[PATCH v2 4/4] banned.h: mark strncpy() as banned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T09:28:28Z","receivedAt":"2018-07-24T09:28:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The strncpy() function is less horrible than strcpy(), but\nis still pretty easy to misuse because of its funny\ntermination semantics. Namely, that if it truncates it omits\nthe NUL terminator, and you must remember to add it\nyourself. Even if you use it correctly, it's sometimes hard\nfor a reader to verify this without hunting through the\ncode. If you're thinking about using it, consider instead:\n\n - strlcpy() if you really just need a truncated but\n NUL-terminated string (we provide a compat version, so\n it's always available)\n\n - xsnprintf() if you're sure that what you're copying\n should fit\n\n - strbuf or xstrfmt() if you need to handle\n arbitrary-length heap-allocated strings\n\nNote that there is one instance of strncpy in\ncompat/regex/regcomp.c, which is fine (it allocates a\nsufficiently large string before copying). But this doesn't\ntrigger the ban-list even when compiling with NO_REGEX=1,\nbecause:\n\n 1. we don't use git-compat-util.h when compiling it\n (instead we rely on the system includes from the\n upstream library); and\n\n 2. It's in an \"#ifdef DEBUG\" block\n\nSince it's doesn't trigger the banned.h code, we're better\noff leaving it to keep our divergence from upstream minimal.\n\nSigned-off-by: Jeff King \n---\n banned.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/banned.h b/banned.h\nindex c83d6cb9df..ab96954583 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -14,6 +14,8 @@\n #define strcpy(x,y) BANNED(strcpy)\n #undef strcat\n #define strcat(x,y) BANNED(strcat)\n+#undef strncpy\n+#define strncpy(x,y,n) BANNED(strncpy)\n \n #undef sprintf\n #undef vsprintf\n-- \n2.18.0.542.g2bf2fc4f7e\n"},{"id":"353497","messageId":"CAPig+cRpcUOA5+k7v3Gy3WsLohedEb=j-a_fCGc3g0ktDfsDVA@mail.gmail.com","threadId":"48922","inReplyTo":"20180724092618.GA3288@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/4] automatically ban strcpy()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-24T17:20:58Z","receivedAt":"2018-07-24T17:21:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 24, 2018 at 5:26 AM Jeff King wrote:\n> 1. We'll only trigger with -Wimplicit-function-declaration\n> (and only stop compilation with -Werror). These are\n> generally enabled by DEVELOPER=1. If you _don't_ have\n> that set, we'll still catch the problem, but only at\n> link-time, with a slightly less useful message:\n>\n> If instead we convert this to a reference to an\n> undefined variable, that always dies immediately. But\n> gcc seems to print the set of errors twice, which\n> clutters things up.\n\nThe above does a pretty good job of convincing me that this ought to\nbe implemented via an undefined variable rather than undefined\nfunction, exactly because it is the newcomer or casual contributor who\nis most likely to trip over a banned function, and almost certainly\nwon't have DEVELOPER=1 set. The gcc clutter seems a minor point\nagainst the benefit this provides to that audience.\n"},{"id":"353615","messageId":"20180726065840.GA27349@sigill.intra.peff.net","threadId":"48922","inReplyTo":"CAPig+cRpcUOA5+k7v3Gy3WsLohedEb=j-a_fCGc3g0ktDfsDVA@mail.gmail.com","subject":"Re: [PATCH v2 1/4] automatically ban strcpy()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-26T06:58:40Z","receivedAt":"2018-07-26T06:58:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 24, 2018 at 01:20:58PM -0400, Eric Sunshine wrote:\n\n> On Tue, Jul 24, 2018 at 5:26 AM Jeff King wrote:\n> > 1. We'll only trigger with -Wimplicit-function-declaration\n> > (and only stop compilation with -Werror). These are\n> > generally enabled by DEVELOPER=1. If you _don't_ have\n> > that set, we'll still catch the problem, but only at\n> > link-time, with a slightly less useful message:\n> >\n> > If instead we convert this to a reference to an\n> > undefined variable, that always dies immediately. But\n> > gcc seems to print the set of errors twice, which\n> > clutters things up.\n> \n> The above does a pretty good job of convincing me that this ought to\n> be implemented via an undefined variable rather than undefined\n> function, exactly because it is the newcomer or casual contributor who\n> is most likely to trip over a banned function, and almost certainly\n> won't have DEVELOPER=1 set. The gcc clutter seems a minor point\n> against the benefit this provides to that audience.\n\nOK. I was on the fence, but it should be pretty trivial to switch. Let\nme see if I can just make a replacement for patch 1, or if the whole\nthing needs to be rebased on top.\n\n-Peff\n"},{"id":"353616","messageId":"20180726072105.GA6057@sigill.intra.peff.net","threadId":"48922","inReplyTo":"20180726065840.GA27349@sigill.intra.peff.net","subject":"[PATCH v3 1/4] automatically ban strcpy()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-26T07:21:05Z","receivedAt":"2018-07-26T07:21:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2018 at 02:58:40AM -0400, Jeff King wrote:\n\n> On Tue, Jul 24, 2018 at 01:20:58PM -0400, Eric Sunshine wrote:\n> \n> > On Tue, Jul 24, 2018 at 5:26 AM Jeff King wrote:\n> > > 1. We'll only trigger with -Wimplicit-function-declaration\n> > > (and only stop compilation with -Werror). These are\n> > > generally enabled by DEVELOPER=1. If you _don't_ have\n> > > that set, we'll still catch the problem, but only at\n> > > link-time, with a slightly less useful message:\n> > >\n> > > If instead we convert this to a reference to an\n> > > undefined variable, that always dies immediately. But\n> > > gcc seems to print the set of errors twice, which\n> > > clutters things up.\n> > \n> > The above does a pretty good job of convincing me that this ought to\n> > be implemented via an undefined variable rather than undefined\n> > function, exactly because it is the newcomer or casual contributor who\n> > is most likely to trip over a banned function, and almost certainly\n> > won't have DEVELOPER=1 set. The gcc clutter seems a minor point\n> > against the benefit this provides to that audience.\n> \n> OK. I was on the fence, but it should be pretty trivial to switch. Let\n> me see if I can just make a replacement for patch 1, or if the whole\n> thing needs to be rebased on top.\n\nThe rest apply cleanly (because they're far enough away textually from\nthe definition of BANNED).\n\nSo here's a replacement for just patch 1 (I'm assuming this creates less\nwork than re-posting them all, but it may not be if Junio prefers\ndealing with a whole new mbox rather than a \"rebase -i\", \"reset --hard\nHEAD^\", \"git am\" -- let me know if you'd prefer it the other way).\n\n-- >8 --\nSubject: [PATCH] automatically ban strcpy()\n\nThere are a few standard C functions (like strcpy) which are\neasy to misuse. E.g.:\n\n char path[PATH_MAX];\n strcpy(path, arg);\n\nmay overflow the \"path\" buffer. Sometimes there's an earlier\nconstraint on the size of \"arg\", but even in such a case\nit's hard to verify that the code is correct. If the size\nreally is unbounded, you're better off using a dynamic\nhelper like strbuf:\n\n struct strbuf path = STRBUF_INIT;\n strbuf_addstr(path, arg);\n\nor if it really is bounded, then use xsnprintf to show your\nexpectation (and get a run-time assertion):\n\n char path[PATH_MAX];\n xsnprintf(path, sizeof(path), \"%s\", arg);\n\nwhich makes further auditing easier.\n\nWe'd usually catch undesirable code like this in a review,\nbut there's no automated enforcement. Adding that\nenforcement can help us be more consistent and save effort\n(and a round-trip) during review.\n\nThis patch teaches the compiler to report an error when it\nsees strcpy (and will become a model for banning a few other\nfunctions). This has a few advantages over a separate\nlinting tool:\n\n 1. We know it's run as part of a build cycle, so it's\n hard to ignore. Whereas an external linter is an extra\n step the developer needs to remember to do.\n\n 2. Likewise, it's basically free since the compiler is\n parsing the code anyway.\n\n 3. We know it's robust against false positives (unlike a\n grep-based linter).\n\nThe two big disadvantages are:\n\n 1. We'll only check code that is actually compiled, so it\n may miss code that isn't triggered on your particular\n system. But since presumably people don't add new code\n without compiling it (and if they do, the banned\n function list is the least of their worries), we really\n only care about failing to clean up old code when\n adding new functions to the list. And that's easy\n enough to address with a manual audit when adding a new\n function (which is what I did for the functions here).\n\n 2. If this ends up generating false positives, it's going\n to be harder to disable (as opposed to a separate\n linter, which may have mechanisms for overriding a\n particular case).\n\n But the intent is to only ban functions which are\n obviously bad, and for which we accept using an\n alternative even when this particular use isn't buggy\n (e.g., the xsnprintf alternative above).\n\nThe implementation here is simple: we'll define a macro for\nthe banned function which replaces it with a reference to a\ndescriptively named but undeclared identifier. Replacing it\nwith any invalid code would work (since we just want to\nbreak compilation). But ideally we'd meet these goals:\n\n - it should be portable; ideally this would trigger\n everywhere, and does not need to be part of a DEVELOPER=1\n setup (because unlike warnings which may depend on the\n compiler or system, this is a clear indicator of\n something wrong in the code).\n\n - it should generate a readable error that gives the\n developer a clue what happened\n\n - it should avoid generating too much other cruft that\n makes it hard to see the actual error\n\n - it should mention the original callsite in the error\n\nThe output with this patch looks like this (using gcc 7, on\na checkout with 022d2ac1f3 reverted, which removed the final\nstrcpy from blame.c):\n\n CC builtin/blame.o\n In file included from ./git-compat-util.h:1246,\n from ./cache.h:4,\n from builtin/blame.c:8:\n builtin/blame.c: In function ‘cmd_blame’:\n ./banned.h:11:22: error: ‘sorry_strcpy_is_a_banned_function’ undeclared (first use in this function)\n #define BANNED(func) sorry_##func##_is_a_banned_function\n ^~~~~~\n ./banned.h:14:21: note: in expansion of macro ‘BANNED’\n #define strcpy(x,y) BANNED(strcpy)\n ^~~~~~\n builtin/blame.c:1074:4: note: in expansion of macro ‘strcpy’\n strcpy(repeated_meta_color, GIT_COLOR_CYAN);\n ^~~~~~\n ./banned.h:11:22: note: each undeclared identifier is reported only once for each function it appears in\n #define BANNED(func) sorry_##func##_is_a_banned_function\n ^~~~~~\n ./banned.h:14:21: note: in expansion of macro ‘BANNED’\n #define strcpy(x,y) BANNED(strcpy)\n ^~~~~~\n builtin/blame.c:1074:4: note: in expansion of macro ‘strcpy’\n strcpy(repeated_meta_color, GIT_COLOR_CYAN);\n ^~~~~~\n\nThis prominently shows the phrase \"strcpy is a banned\nfunction\", along with the original callsite in blame.c and\nthe location of the ban code in banned.h. Which should be\nenough to get even a developer seeing this for the first\ntime pointed in the right direction.\n\nThis doesn't match our ideals perfectly, but it's a pretty\ngood balance. A few alternatives I tried:\n\n 1. Instead of using an undeclared variable, using an\n undeclared function. This shortens the message, because\n the \"each undeclared identifier\" message is not needed\n (and as you can see above, it triggers a separate\n mention of each of the expansion points).\n\n But it doesn't actually stop compilation unless you use\n -Werror=implicit-function-declaration in your CFLAGS.\n This is the case for DEVELOPER=1, but not for a default\n build (on the other hand, we'd eventually produce a\n link error pointing to the correct source line with the\n descriptive name).\n\n 2. The linux kernel uses a similar mechanism in its\n BUILD_BUG_ON_MSG(), where they actually declare the\n function but do so with gcc's error attribute. But\n that's not portable to other compilers (and it also\n runs afoul of our error() macro).\n\n We could make a gcc-specific technique and fallback on\n other compilers, but it's probably not worth the\n complexity. It also isn't significantly shorter than\n the error message shown above.\n\n 3. We could drop the BANNED() macro, which would shorten\n the number of lines in the error. But curiously,\n removing it (and just expanding strcpy directly to the\n bogus identifier) causes gcc _not_ to report the\n original line of code.\n\nSo this strategy seems to be an acceptable mix of\ninformation, portability, simplicity, and robustness,\nwithout _too_ much extra clutter. I also tested it with\nclang, and it looks as good (actually, slightly less\ncluttered than with gcc).\n\nSigned-off-by: Jeff King \n---\n banned.h | 16 ++++++++++++++++\n git-compat-util.h | 6 ++++++\n 2 files changed, 22 insertions(+)\n create mode 100644 banned.h\n\ndiff --git a/banned.h b/banned.h\nnew file mode 100644\nindex 0000000000..1a3e526570\n--- /dev/null\n+++ b/banned.h\n@@ -0,0 +1,16 @@\n+#ifndef BANNED_H\n+#define BANNED_H\n+\n+/*\n+ * This header lists functions that have been banned from our code base,\n+ * because they're too easy to misuse (and even if used correctly,\n+ * complicate audits). Including this header turns them into compile-time\n+ * errors.\n+ */\n+\n+#define BANNED(func) sorry_##func##_is_a_banned_function\n+\n+#undef strcpy\n+#define strcpy(x,y) BANNED(strcpy)\n+\n+#endif /* BANNED_H */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9a64998b24..89d37095c7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1239,4 +1239,10 @@ extern void unleak_memory(const void *ptr, size_t len);\n #define UNLEAK(var) do {} while (0)\n #endif\n \n+/*\n+ * This include must come after system headers, since it introduces macros that\n+ * replace system names.\n+ */\n+#include \"banned.h\"\n+\n #endif\n-- \n2.18.0.795.g7103b4bd43\n\n"},{"id":"353663","messageId":"xmqqfu05rjfc.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180726072105.GA6057@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] automatically ban strcpy()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-26T17:33:27Z","receivedAt":"2018-07-26T17:33:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So here's a replacement for just patch 1 (I'm assuming this creates less\n> work than re-posting them all, but it may not be if Junio prefers\n> dealing with a whole new mbox rather than a \"rebase -i\", \"reset --hard\n> HEAD^\", \"git am\" -- let me know if you'd prefer it the other way).\n\nA single patch replacement that is clearly marked which one to\nreplace and which other ones to keep, like you did here, is fine.\nThe amount of work is about the same either way.\n\n0) I would first do these to make sure that I can replace:\n\n $ git checkout jk/banned-functions\n $ git log --first-parent --oneline master..\n $ git log --first-parent --oneline next..\n\nIf 'next' has some patches that are not in 'master' from the topic,\nI must refrain from replacing them (in this case, there is no such\nissue).\n\n1-a) With a wholesale replacement,\n\n $ git checkout master...\t;# try to keep the same base\n $ git am -s mbox\t\t;# on detached HEAD\n $ git tbdiff ..@{-1} @{-1}..\t;# or range-diff\n\nIf the range-/tbdiff tells me that earlier N patches are the same,\nthe above is followed by something like (pardon off-by-one)\n\n $ git rebase --onto @{-1}~N HEAD~N\n\nto preserve as many original commits as possible.\n\n1-b) With a single patch replacement, it is quite different.\n\n $ git checkout HEAD~4\t ;# we are replacing 1/4 of the original\n $ git am -s mbox ;# that single patch\n $ git show-branch HEAD @{-1}\n\nThat would give me something like this\n\n * [HEAD] automatically ban strcpy()\n ! [@{-1}] banned.h: mark strncpy() as banned\n --\n * [HEAD] automatically ban strcpy()\n + [@{-1}] banned.h: mark strncpy() as banned\n + [@{-1}^] banned.h: mark sprintf() as banned\n + [@{-1}~2] banned.h: mark strcat() as banned\n + [@{-1}~3] automatically ban strcpy()\n -- [HEAD^] Merge branch 'sb/blame-color' into jk/banned-function\n\nThe most natural thing to do at this point is\n\n $ git cherry-pick -3 @{-1}\n\nBut we know range-pick is buggy and loses core.rewriteref, so\ninstead I did this, which I know carries the notes forward:\n\n $ git rebase --onto HEAD @{-1}~3 @{-1}^0\n\n Side note: The point of last \"^0\" is that I do not want to lose\n the topic branch jk/banned-functions not just yet.\n\nIf I need to re-apply separate pieces of the original history, it\nbecomes very painful to emulate these multiple cherry-picks with\nmultiple \"rebase --onto\", though. That is where \"the amount of work\nis about the same\" comes from. If cherry-pick worked correctly,\nselective replacement should be less work for me.\n\nAnyway, we already preserved as many original commits, but unlike\n1-a, did so manually when we decided to replace selective patches.\nSo there is no further rebasing with this approach.\n\n2) I now have two diverged histories in HEAD and @{-1} that I can\ncompare with range-/tbdiff in either case: \n\n $ git tbdiff ..@{-1} @{-1}..\n\nAfter the usual inspection and testing, replacement is concluded by\n\n $ git checkout -B @{-1}\n\nwhich takes me back to (an updated) jk/banned-functions topic.\n\n"},{"id":"353690","messageId":"20180727080807.GA11932@sigill.intra.peff.net","threadId":"48922","inReplyTo":"xmqqfu05rjfc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/4] automatically ban strcpy()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-27T08:08:08Z","receivedAt":"2018-07-27T08:08:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2018 at 10:33:27AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So here's a replacement for just patch 1 (I'm assuming this creates less\n> > work than re-posting them all, but it may not be if Junio prefers\n> > dealing with a whole new mbox rather than a \"rebase -i\", \"reset --hard\n> > HEAD^\", \"git am\" -- let me know if you'd prefer it the other way).\n> \n> A single patch replacement that is clearly marked which one to\n> replace and which other ones to keep, like you did here, is fine.\n> The amount of work is about the same either way.\n> \n> 0) I would first do these to make sure that I can replace:\n> [..]\n\nThanks. As always, I find it interesting to see your workflows.\n\n> 1-b) With a single patch replacement, it is quite different.\n> \n> $ git checkout HEAD~4\t ;# we are replacing 1/4 of the original\n> $ git am -s mbox ;# that single patch\n> $ git show-branch HEAD @{-1}\n> [...]\n> The most natural thing to do at this point is\n> \n> $ git cherry-pick -3 @{-1}\n> \n> But we know range-pick is buggy and loses core.rewriteref, so\n> instead I did this, which I know carries the notes forward:\n> \n> $ git rebase --onto HEAD @{-1}~3 @{-1}^0\n\nInteresting. I'd have probably done it with an interactive rebase:\n\n $ git rebase -i HEAD~4\n [change first \"pick\" to \"edit\"; after stopping...]\n $ git reset --hard HEAD^ ;# throw away patch 1\n $ git am -s mbox ;# apply single patch\n $ git rebase --continue\n\nWhich is really the same thing, but \"cheats\" around the cherry-pick\nproblem by using rebase (which I think handles the rewriteref stuff\ncorrectly even in interactive mode).\n\nI guess if we wanted to be really fancy, just replacing the first \"pick\"\nwith \"x git am -s mbox\" would automate it. That might be handy for the\nmulti-patch case.\n\nAnyway, thanks for handling it. :)\n\n-Peff\n"},{"id":"353734","messageId":"xmqq8t5woa5f.fsf@gitster-ct.c.googlers.com","threadId":"48922","inReplyTo":"20180727080807.GA11932@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] automatically ban strcpy()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T17:34:20Z","receivedAt":"2018-07-27T17:34:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> $ git rebase --onto HEAD @{-1}~3 @{-1}^0\n>\n> Interesting. I'd have probably done it with an interactive rebase:\n>\n> $ git rebase -i HEAD~4\n> [change first \"pick\" to \"edit\"; after stopping...]\n> $ git reset --hard HEAD^ ;# throw away patch 1\n> $ git am -s mbox ;# apply single patch\n> $ git rebase --continue\n>\n> Which is really the same thing,...\n\nI have unfounded fear for doing anything other than \"edit\", \"commit\n--amend\", \"rebase --continue\", or \"rebase --abort\" during a \"rebase\n-i\" session. \n\nEspeciallly \"reset --hard\" with anything but HEAD. I guess that is\nbecause I do not fully trust/understand how the sequencer machinery\nreplays the remainder of todo tasks on top of the HEAD that is\ndifferent from what it left me to futz with when it relinquished the\ncontrol.\n\nAlso \"am\" during \"rebase -i\" is scary to me, as \"am\" also tries to\nkeep its own sequencing state. Do you know if \"rebase --continue\"\nwould work correctly in the above sequence if \"am\" conflicted, I\ngave up, and said \"am --abort\", for example? I don't offhand know.\n"},{"id":"353792","messageId":"20180728092443.GB16454@sigill.intra.peff.net","threadId":"48922","inReplyTo":"xmqq8t5woa5f.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/4] automatically ban strcpy()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-28T09:24:43Z","receivedAt":"2018-07-28T09:24:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 27, 2018 at 10:34:20AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> $ git rebase --onto HEAD @{-1}~3 @{-1}^0\n> >\n> > Interesting. I'd have probably done it with an interactive rebase:\n> >\n> > $ git rebase -i HEAD~4\n> > [change first \"pick\" to \"edit\"; after stopping...]\n> > $ git reset --hard HEAD^ ;# throw away patch 1\n> > $ git am -s mbox ;# apply single patch\n> > $ git rebase --continue\n> >\n> > Which is really the same thing,...\n> \n> I have unfounded fear for doing anything other than \"edit\", \"commit\n> --amend\", \"rebase --continue\", or \"rebase --abort\" during a \"rebase\n> -i\" session. \n> \n> Especiallly \"reset --hard\" with anything but HEAD. I guess that is\n> because I do not fully trust/understand how the sequencer machinery\n> replays the remainder of todo tasks on top of the HEAD that is\n> different from what it left me to futz with when it relinquished the\n> control.\n\nI jump around via \"reset --hard\" all the time during interactive\nrebases. I don't recall having any issues, though that does not mean\nthere isn't a corner case lurking. :)\n\n> Also \"am\" during \"rebase -i\" is scary to me, as \"am\" also tries to\n> keep its own sequencing state. Do you know if \"rebase --continue\"\n> would work correctly in the above sequence if \"am\" conflicted, I\n> gave up, and said \"am --abort\", for example? I don't offhand know.\n\nThis one is trickier. I _assumed_ it would be fine to \"am\" during a\nrebase, but I didn't actually try it (in fact, I rarely do anything\nexotic with \"am\", since my main use is just test-applying patches on a\ndetached head).\n\nIt _seems_ to work with this example:\n\n-- >8 --\ngit init repo\ncd repo\n\nfor i in 1 2 3 4; do\n echo $i >file\n git add file\n git commit -m $i\ndone\ngit format-patch --stdout -1 >patch\n\ngit reset --hard HEAD^\nGIT_EDITOR='perl -i -pe \"/2$/ and s/pick/edit/\"' git rebase -i HEAD~2\ngit am patch\ngit am --abort\n-- 8< --\n\nI think because \"am\" lives in $GIT_DIR/rebase-apply, and \"rebase -i\"\nlives in \".git/rebase-merge\". Of course \"rebase\" can use the\nrebase-apply directory, but I think interactive-rebase never will.\n\nSo it works, but mostly by luck. :)\n\nIn my ideal world, operations like this that can be continued would be\nstored in a stack, and each stack element would know its operation type.\nSo you could do:\n\n # push a rebase onto the stack\n git rebase foo\n\n # while stopped, you might do another operation which pushes onto the\n # stack\n git am ~/patch\n\n # aborting an operation (or finishing it naturally) pops it off the\n # stack; now we just have the rebase on the stack\n git am --abort\n\n # aborting an operation that's not at the top of the stack would\n # either be an error, or could auto-abort everybody on top\n git am ~/patch\n git rebase --abort ;# aborts the am, too!\n\n # you could even nest similar operations; by default we find the\n # top-most one in the stack, but you could refer to them by stack\n # position.\n #\n # put us in a rebase that stops at a conflict\n git rebase foo\n # oops, rewrite the last few commits as part of fixing the conflict\n git rebase -i HEAD~3\n # nope, let's abort the whole thing (stack level 0)\n git rebase --abort=0\n\n # it would also be nice to have generic commands to manipulate the\n # stack\n git op list ;# show the stack\n git op abort ;# abort the top operation, whatever it is\n git op continue ;# continue the top operation, whatever it is\n\nI've hacked something similar to \"git op continue\" myself and find it\nvery useful, but:\n\n - it's intimately aware of all the possible operations, including some\n custom ones that I have. I wouldn't need to if each operation\n touched a well-known directory to push itself on the stack, and\n provided a few commands in the stack directory for things like \"run\n this to abort me\".\n\n - it sometimes behaves weirdly, because I \"canceled\" an operation with\n \"reset\" or \"checkout\", and later I expect to continue operation X,\n but find that some other operation Y was waiting. Having a stack\n means it's much easier to see which operations are still hanging\n around (we have this already with the prompt logic that shows things\n like rebases, but again, it has to be intimate with which operations\n are storing data in $GIT_DIR)\n\nAnyway. That's something I've dreamed about for a while, but the thought\nof retro-fitting the existing multi-command operations turned me off.\nThe current systems _usually_ works.\n\n-Peff\n"}]}