threads / patch / 48883

patchblame: prefer xsnprintf to strcpy for colors

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

## tl;dr

5 messages between Jul 13, 2018 and Jul 13, 2018. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jeff King· Jul 13, 2018, 20:43 UTC · lore

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(-)
Show changes to builtin/blame.c +3 −1
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
Stefan Beller· Jul 13, 2018, 20:58 UTC · re: Jeff King · lore

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

On Fri, Jul 13, 2018 at 1:43 PM Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
> ---
> 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?

Jeff King· Jul 13, 2018, 21:04 UTC · re: Stefan Beller · lore

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

On Fri, Jul 13, 2018 at 01:58:05PM -0700, Stefan Beller wrote:
Show 6 quoted lines
> > 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
Stefan Beller· Jul 13, 2018, 21:10 UTC · re: Jeff King · lore

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

On Fri, Jul 13, 2018 at 2:04 PM Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
>
> 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")"? :-)
Show 7 quoted lines
> 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
Jeff King· Jul 13, 2018, 21:29 UTC · re: Stefan Beller · lore

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

On Fri, Jul 13, 2018 at 02:10:24PM -0700, Stefan Beller wrote:
Show 6 quoted lines
> > 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.
Show 12 quoted lines
> > 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

← back to recent threads