[PATCH] blame: prefer xsnprintf to strcpy for colors

[PATCH] blame: prefer xsnprintf to strcpy for colors

[Jeff King]

Our color buffers are all COLOR_MAXLEN, which fits the
largest possible color. So we can never overflow the buffer
by copying an existing color. However, using strcpy() makes
it harder to audit the code-base for calls that _are_
problems. We should use something like xsnprintf(), which
shows the reader that we expect this never to fail (and
provides a run-time assertion if it does, just in case).

Signed-off-by: Jeff King <peff@peff.net>
---
Another option would just be color_parse(repeated_meta_color, "cyan").
The run-time cost is slightly higher, but it probably doesn't matter
here, and perhaps it's more readable.

This is a repost from:

  https://public-inbox.org/git/20180610204419.GA11273@sigill.intra.peff.net/

which I think just got overlooked as we were in the midst of the 2.18
release cycle.

 builtin/blame.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 5a0388aaef..63bdf755bb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1069,7 +1069,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		find_alignment(&sb, &output_option);
 		if (!*repeated_meta_color &&
 		    (output_option & OUTPUT_COLOR_LINE))
-			strcpy(repeated_meta_color, GIT_COLOR_CYAN);
+			xsnprintf(repeated_meta_color,
+				  sizeof(repeated_meta_color),
+				  "%s", GIT_COLOR_CYAN);
 	}
 	if (output_option & OUTPUT_ANNOTATE_COMPAT)
 		output_option &= ~(OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR);
-- 
2.18.0.433.gb9621797ee

Re: [PATCH] blame: prefer xsnprintf to strcpy for colors

[Stefan Beller]

On Fri, Jul 13, 2018 at 1:43 PM Jeff King <peff@peff.net> wrote:

> ---
> Another option would just be color_parse(repeated_meta_color, "cyan").
> The run-time cost is slightly higher, but it probably doesn't matter
> here, and perhaps it's more readable.
>

Thanks for posting this again; this looks good to me!
Stefan

> I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid
> that function altogether, even when we _think_ it can't trigger an
> overflow. That's easier to reason about (and makes auditing easier).

Can we somehow automatically find "bad code" either in pathces
or in new code (such as pu), e.g. as a coccicheck for these functions?

Re: [PATCH] blame: prefer xsnprintf to strcpy for colors

[Jeff King]

On Fri, Jul 13, 2018 at 01:58:05PM -0700, Stefan Beller wrote:

> > I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid
> > that function altogether, even when we _think_ it can't trigger an
> > overflow. That's easier to reason about (and makes auditing easier).
> 
> Can we somehow automatically find "bad code" either in pathces
> or in new code (such as pu), e.g. as a coccicheck for these functions?

I'd be happy to declare strcpy() totally banned (and it more or less
is). I found this with a simple "git grep", though it seems like a
trivial application of coccinelle to find it. The question is what to
convert it into. xsnprintf() is often a good choice, but not always
(e.g., if the destination isn't an array, we'd have to get the size from
somewhere else).

I wouldn't be surprised if there's a way to ask coccinelle to convert
the easy cases and barf with an error on the hard cases or something. I
don't know the tool very well.

-Peff

Re: [PATCH] blame: prefer xsnprintf to strcpy for colors

[Stefan Beller]

On Fri, Jul 13, 2018 at 2:04 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Jul 13, 2018 at 01:58:05PM -0700, Stefan Beller wrote:
>
> > > I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid
> > > that function altogether, even when we _think_ it can't trigger an
> > > overflow. That's easier to reason about (and makes auditing easier).
> >
> > Can we somehow automatically find "bad code" either in pathces
> > or in new code (such as pu), e.g. as a coccicheck for these functions?
>
> I'd be happy to declare strcpy() totally banned (and it more or less
> is). I found this with a simple "git grep", though it seems like a
> trivial application of coccinelle to find it. The question is what to
> convert it into.

into some "meta BUG("your review process failed")"? :-)

> xsnprintf() is often a good choice, but not always
> (e.g., if the destination isn't an array, we'd have to get the size from
> somewhere else).
>
> I wouldn't be surprised if there's a way to ask coccinelle to convert
> the easy cases and barf with an error on the hard cases or something. I
> don't know the tool very well.

I was just suggesting that tool as it is run on pu by some automation,
hence it would not fall through the cracks. I mean how often to you
happen to run git grep looking for strcpy on our code base and do we
want to rely on that in the long run?

Stefan

Re: [PATCH] blame: prefer xsnprintf to strcpy for colors

[Jeff King]

On Fri, Jul 13, 2018 at 02:10:24PM -0700, Stefan Beller wrote:

> > I'd be happy to declare strcpy() totally banned (and it more or less
> > is). I found this with a simple "git grep", though it seems like a
> > trivial application of coccinelle to find it. The question is what to
> > convert it into.
> 
> into some "meta BUG("your review process failed")"? :-)

Heh, yes, I was tempted to suggest that.

> > xsnprintf() is often a good choice, but not always
> > (e.g., if the destination isn't an array, we'd have to get the size from
> > somewhere else).
> >
> > I wouldn't be surprised if there's a way to ask coccinelle to convert
> > the easy cases and barf with an error on the hard cases or something. I
> > don't know the tool very well.
> 
> I was just suggesting that tool as it is run on pu by some automation,
> hence it would not fall through the cracks. I mean how often to you
> happen to run git grep looking for strcpy on our code base and do we
> want to rely on that in the long run?

Clearly not often enough. :)

I probably do it once or twice a year, but the ideal is "as part of
testing every topic". It wouldn't be hard to script around "git grep" as
part of the DEVELOPER build), but I think that still turns up some false
positives. Not for strcpy() or sprintf(), but snprintf() for example is
easy to use badly but sometimes the correct tool. We also have an
strncpy() which would be easy to turn into memcpy(), but it's in the
compat/regex code, which preferably we wouldn't change.

There are also probably better tools than grep (i.e., that actually
parse C), but they may not be worth the overhead (though if we can reuse
cocci for this, that seems easy enough).

As an aside, I recently got introduced to Microsoft's SDL Banned
Function list:

  https://msdn.microsoft.com/en-us/library/bb288454.aspx

They even hate memcpy! I'll grant that you _can_ use memcpy badly, but
it's often the right tool (and IMHO the C11 Annex.K memcpy_s() is not
much better).

-Peff