{"thread":{"id":"48883","subject":"[PATCH] blame: prefer xsnprintf to strcpy for colors","startedAt":"2018-07-13T20:43:53Z","lastAt":"2018-07-13T21:29:37Z","messageCount":5,"participants":["Jeff King","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352537","messageId":"20180713204350.GA16999@sigill.intra.peff.net","threadId":"48883","inReplyTo":null,"subject":"[PATCH] blame: prefer xsnprintf to strcpy for colors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T20:43:50Z","receivedAt":"2018-07-13T20:43:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our color buffers are all COLOR_MAXLEN, which fits the\nlargest possible color. So we can never overflow the buffer\nby copying an existing color. However, using strcpy() makes\nit harder to audit the code-base for calls that _are_\nproblems. We should use something like xsnprintf(), which\nshows the reader that we expect this never to fail (and\nprovides a run-time assertion if it does, just in case).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnother option would just be color_parse(repeated_meta_color, \"cyan\").\nThe run-time cost is slightly higher, but it probably doesn't matter\nhere, and perhaps it's more readable.\n\nThis is a repost from:\n\n  https://public-inbox.org/git/20180610204419.GA11273@sigill.intra.peff.net/\n\nwhich I think just got overlooked as we were in the midst of the 2.18\nrelease cycle.\n\n builtin/blame.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 5a0388aaef..63bdf755bb 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1069,7 +1069,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tfind_alignment(&sb, &output_option);\n \t\tif (!*repeated_meta_color &&\n \t\t    (output_option & OUTPUT_COLOR_LINE))\n-\t\t\tstrcpy(repeated_meta_color, GIT_COLOR_CYAN);\n+\t\t\txsnprintf(repeated_meta_color,\n+\t\t\t\t  sizeof(repeated_meta_color),\n+\t\t\t\t  \"%s\", GIT_COLOR_CYAN);\n \t}\n \tif (output_option & OUTPUT_ANNOTATE_COMPAT)\n \t\toutput_option &= ~(OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR);\n-- \n2.18.0.433.gb9621797ee\n"},{"id":"352538","messageId":"CAGZ79kakrS_tk=x4Rb9qSu9e4RVSJ=J64eOdasKhOy=g8kV68w@mail.gmail.com","threadId":"48883","inReplyTo":"20180713204350.GA16999@sigill.intra.peff.net","subject":"Re: [PATCH] blame: prefer xsnprintf to strcpy for colors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-13T20:58:05Z","receivedAt":"2018-07-13T20:58:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 13, 2018 at 1:43 PM Jeff King <peff@peff.net> wrote:\n\n> ---\n> Another option would just be color_parse(repeated_meta_color, \"cyan\").\n> The run-time cost is slightly higher, but it probably doesn't matter\n> here, and perhaps it's more readable.\n>\n\nThanks for posting this again; this looks good to me!\nStefan\n\n> I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid\n> that function altogether, even when we _think_ it can't trigger an\n> overflow. That's easier to reason about (and makes auditing easier).\n\nCan we somehow automatically find \"bad code\" either in pathces\nor in new code (such as pu), e.g. as a coccicheck for these functions?\n"},{"id":"352539","messageId":"20180713210452.GA18909@sigill.intra.peff.net","threadId":"48883","inReplyTo":"CAGZ79kakrS_tk=x4Rb9qSu9e4RVSJ=J64eOdasKhOy=g8kV68w@mail.gmail.com","subject":"Re: [PATCH] blame: prefer xsnprintf to strcpy for colors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T21:04:53Z","receivedAt":"2018-07-13T21:04:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 13, 2018 at 01:58:05PM -0700, Stefan Beller wrote:\n\n> > I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid\n> > that function altogether, even when we _think_ it can't trigger an\n> > overflow. That's easier to reason about (and makes auditing easier).\n> \n> Can we somehow automatically find \"bad code\" either in pathces\n> or in new code (such as pu), e.g. as a coccicheck for these functions?\n\nI'd be happy to declare strcpy() totally banned (and it more or less\nis). I found this with a simple \"git grep\", though it seems like a\ntrivial application of coccinelle to find it. The question is what to\nconvert it into. xsnprintf() is often a good choice, but not always\n(e.g., if the destination isn't an array, we'd have to get the size from\nsomewhere else).\n\nI wouldn't be surprised if there's a way to ask coccinelle to convert\nthe easy cases and barf with an error on the hard cases or something. I\ndon't know the tool very well.\n\n-Peff\n"},{"id":"352540","messageId":"CAGZ79kYpZy4q4vD3z5oZNZYgCYWu4YWeA4DX=E8jFW9=XUe7_g@mail.gmail.com","threadId":"48883","inReplyTo":"20180713210452.GA18909@sigill.intra.peff.net","subject":"Re: [PATCH] blame: prefer xsnprintf to strcpy for colors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-13T21:10:24Z","receivedAt":"2018-07-13T21:10:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 13, 2018 at 2:04 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Jul 13, 2018 at 01:58:05PM -0700, Stefan Beller wrote:\n>\n> > > I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid\n> > > that function altogether, even when we _think_ it can't trigger an\n> > > overflow. That's easier to reason about (and makes auditing easier).\n> >\n> > Can we somehow automatically find \"bad code\" either in pathces\n> > or in new code (such as pu), e.g. as a coccicheck for these functions?\n>\n> I'd be happy to declare strcpy() totally banned (and it more or less\n> is). I found this with a simple \"git grep\", though it seems like a\n> trivial application of coccinelle to find it. The question is what to\n> convert it into.\n\ninto some \"meta BUG(\"your review process failed\")\"? :-)\n\n> xsnprintf() is often a good choice, but not always\n> (e.g., if the destination isn't an array, we'd have to get the size from\n> somewhere else).\n>\n> I wouldn't be surprised if there's a way to ask coccinelle to convert\n> the easy cases and barf with an error on the hard cases or something. I\n> don't know the tool very well.\n\nI was just suggesting that tool as it is run on pu by some automation,\nhence it would not fall through the cracks. I mean how often to you\nhappen to run git grep looking for strcpy on our code base and do we\nwant to rely on that in the long run?\n\nStefan\n"},{"id":"352541","messageId":"20180713212934.GA19565@sigill.intra.peff.net","threadId":"48883","inReplyTo":"CAGZ79kYpZy4q4vD3z5oZNZYgCYWu4YWeA4DX=E8jFW9=XUe7_g@mail.gmail.com","subject":"Re: [PATCH] blame: prefer xsnprintf to strcpy for colors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-13T21:29:34Z","receivedAt":"2018-07-13T21:29:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 13, 2018 at 02:10:24PM -0700, Stefan Beller wrote:\n\n> > I'd be happy to declare strcpy() totally banned (and it more or less\n> > is). I found this with a simple \"git grep\", though it seems like a\n> > trivial application of coccinelle to find it. The question is what to\n> > convert it into.\n> \n> into some \"meta BUG(\"your review process failed\")\"? :-)\n\nHeh, yes, I was tempted to suggest that.\n\n> > xsnprintf() is often a good choice, but not always\n> > (e.g., if the destination isn't an array, we'd have to get the size from\n> > somewhere else).\n> >\n> > I wouldn't be surprised if there's a way to ask coccinelle to convert\n> > the easy cases and barf with an error on the hard cases or something. I\n> > don't know the tool very well.\n> \n> I was just suggesting that tool as it is run on pu by some automation,\n> hence it would not fall through the cracks. I mean how often to you\n> happen to run git grep looking for strcpy on our code base and do we\n> want to rely on that in the long run?\n\nClearly not often enough. :)\n\nI probably do it once or twice a year, but the ideal is \"as part of\ntesting every topic\". It wouldn't be hard to script around \"git grep\" as\npart of the DEVELOPER build), but I think that still turns up some false\npositives. Not for strcpy() or sprintf(), but snprintf() for example is\neasy to use badly but sometimes the correct tool. We also have an\nstrncpy() which would be easy to turn into memcpy(), but it's in the\ncompat/regex code, which preferably we wouldn't change.\n\nThere are also probably better tools than grep (i.e., that actually\nparse C), but they may not be worth the overhead (though if we can reuse\ncocci for this, that seems easy enough).\n\nAs an aside, I recently got introduced to Microsoft's SDL Banned\nFunction list:\n\n  https://msdn.microsoft.com/en-us/library/bb288454.aspx\n\nThey even hate memcpy! I'll grant that you _can_ use memcpy badly, but\nit's often the right tool (and IMHO the C11 Annex.K memcpy_s() is not\nmuch better).\n\n-Peff\n"}]}