threads / patch / 41373

patchtest-path-utils: use xsnprintf in favor of strcpy

Subject: [PATCH] test-path-utils: use xsnprintf in favor of strcpy

## tl;dr

7 messages between Feb 8, 2016 and Feb 9, 2016. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Jeff King· Feb 8, 2016, 22:21 UTC · lore

This strcpy will never overflow because it's copying from baked-in test data. But we would prefer to avoid strcpy entirely, as it makes it harder to audit for real security bugs.

Signed-off-by: Jeff King <peff@peff.net>
---
Repost of <20160114202608.GA8806@sigill.intra.peff.net> from a few weeks
ago (sorry, gmane is down so I can't generate a link). I think the
original was never applied because the topic that introduced the strcpy
(js/dirname-basename) predated xsnprintf, so there was some merging
complexity. Now that topic is in master, so this can be applied there.
 test-path-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to test-path-utils.c +1 −1
diff --git a/test-path-utils.c b/test-path-utils.c
index c3adcd8..6232dfe 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -56,7 +56,7 @@ static int test_function(struct test_data *data, char *(*func)(char *input),
 		if (!data[i].from)
 			to = func(NULL);
 		else {
-			strcpy(buffer, data[i].from);
+			xsnprintf(buffer, sizeof(buffer), "%s", data[i].from);
 			to = func(buffer);
 		}
 		if (!strcmp(to, data[i].to))
-- 
2.7.1.526.gd04f550
Jeff King· Feb 8, 2016, 22:25 UTC · re: Jeff King · lore

[PATCH] rerere: replace strcpy with xsnprintf

This shouldn't overflow, as we are copying a sha1 hex into a 41-byte buffer. But it does not hurt to use a bound-checking function, which protects us and makes auditing for overflows easier.

Signed-off-by: Jeff King <peff@peff.net>
---
These strcpy calls go away in jc/rerere-multi, so I was holding onto
this to see if that graduated. But since that is stalled, I figured it
cannot hurt to post (and the conflict resolution is obviously trivial).

With this and the previous patch, it makes our code base strcpy free. Yay.

 rerere.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to rerere.c +2 −2
diff --git a/rerere.c b/rerere.c
index 403c700..587b7e2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -48,7 +48,7 @@ static int has_rerere_resolution(const struct rerere_id *id)
 static struct rerere_id *new_rerere_id_hex(char *hex)
 {
 	struct rerere_id *id = xmalloc(sizeof(*id));
-	strcpy(id->hex, hex);
+	xsnprintf(id->hex, sizeof(id->hex), "%s", hex);
 	return id;
 }
 
@@ -904,7 +904,7 @@ int rerere_forget(struct pathspec *pathspec)
 static struct rerere_id *dirname_to_id(const char *name)
 {
 	static struct rerere_id id;
-	strcpy(id.hex, name);
+	xsnprintf(id.hex, sizeof(id.hex), "%s", name);
 	return &id;
 }
 
-- 
2.7.1.526.gd04f550
Junio C Hamano· Feb 8, 2016, 23:07 UTC · re: Jeff King · lore

Re: [PATCH] rerere: replace strcpy with xsnprintf

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> This shouldn't overflow, as we are copying a sha1 hex into a
> 41-byte buffer. But it does not hurt to use a bound-checking
> function, which protects us and makes auditing for overflows
> easier.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> These strcpy calls go away in jc/rerere-multi, so I was holding onto
> this to see if that graduated. But since that is stalled, I figured it
> cannot hurt to post (and the conflict resolution is obviously trivial).
>
> With this and the previous patch, it makes our code base strcpy free.
> Yay.
Thanks.  I think jc/rerere-multi can be rerolled on top of this.
Show 25 quoted lines
>
>  rerere.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index 403c700..587b7e2 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -48,7 +48,7 @@ static int has_rerere_resolution(const struct rerere_id *id)
>  static struct rerere_id *new_rerere_id_hex(char *hex)
>  {
>  	struct rerere_id *id = xmalloc(sizeof(*id));
> -	strcpy(id->hex, hex);
> +	xsnprintf(id->hex, sizeof(id->hex), "%s", hex);
>  	return id;
>  }
>  
> @@ -904,7 +904,7 @@ int rerere_forget(struct pathspec *pathspec)
>  static struct rerere_id *dirname_to_id(const char *name)
>  {
>  	static struct rerere_id id;
> -	strcpy(id.hex, name);
> +	xsnprintf(id.hex, sizeof(id.hex), "%s", name);
>  	return &id;
>  }
Junio C Hamano· Feb 8, 2016, 22:41 UTC · re: Jeff King · lore

Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> This strcpy will never overflow because it's copying from
> baked-in test data. But we would prefer to avoid strcpy
> entirely, as it makes it harder to audit for real security
> bugs.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Repost of <20160114202608.GA8806@sigill.intra.peff.net> from a few weeks
> ago (sorry, gmane is down so I can't generate a link). I think the
> original was never applied because the topic that introduced the strcpy
> (js/dirname-basename) predated xsnprintf, so there was some merging
> complexity. Now that topic is in master, so this can be applied there.

Thanks. This kind of considerate "holding it off for a few weeks" helps things a lot.

Show 17 quoted lines
>
>  test-path-utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test-path-utils.c b/test-path-utils.c
> index c3adcd8..6232dfe 100644
> --- a/test-path-utils.c
> +++ b/test-path-utils.c
> @@ -56,7 +56,7 @@ static int test_function(struct test_data *data, char *(*func)(char *input),
>  		if (!data[i].from)
>  			to = func(NULL);
>  		else {
> -			strcpy(buffer, data[i].from);
> +			xsnprintf(buffer, sizeof(buffer), "%s", data[i].from);
>  			to = func(buffer);
>  		}
>  		if (!strcmp(to, data[i].to))
Eric Wong· Feb 8, 2016, 23:07 UTC · re: Jeff King · lore

Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy

Jeff King <peff@peff.net> wrote:
> Repost of <20160114202608.GA8806@sigill.intra.peff.net> from a few weeks
> ago (sorry, gmane is down so I can't generate a link).

I prefer we use links derived from Message-IDs anyways. This prevents reliance on gmane article numbers being a central point of failure:

http://marc.info/?i=$MESSAGE_ID http://mid.gmane.org/$MESSAGE_ID http://mid.mail-archive.com/$MESSAGE_ID

But the MESSAGE_ID above seems missing from mail-archive.com, in this case.
Maybe there's more lookup-by-Message-ID services out there...
Jeff King· Feb 8, 2016, 23:13 UTC · re: Eric Wong · lore

Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy

On Mon, Feb 08, 2016 at 11:07:26PM +0000, Eric Wong wrote:
Show 11 quoted lines
> Jeff King <peff@peff.net> wrote:
> > Repost of <20160114202608.GA8806@sigill.intra.peff.net> from a few weeks
> > ago (sorry, gmane is down so I can't generate a link).
> 
> I prefer we use links derived from Message-IDs anyways.  This
> prevents reliance on gmane article numbers being a central point
> of failure:
> 
> http://marc.info/?i=$MESSAGE_ID
> http://mid.gmane.org/$MESSAGE_ID
> http://mid.mail-archive.com/$MESSAGE_ID

I actually do, too. I keep a local archive of the whole list, and I have a script that hits gmane to convert their article ids into message-ids. When gmane is down I can still use my archive, but I can't resolve anybody's article mentions. :)

I mostly use gmane links because people are used to them, though (I also don't think there's a way using message-ids to point to a whole thread with an article highlighted, though of course readers can get their by clicking through).

> But the MESSAGE_ID above seems missing from mail-archive.com, in this case.

It seems to have a giant hole in git@vger messages between 2016-01-07 and 2016-01-20, which covers the referenced message (which was on the 14th).

-Peff
Johannes Schindelin· Feb 9, 2016, 10:13 UTC · re: Jeff King · lore

Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy

Hi Peff,
On Mon, 8 Feb 2016, Jeff King wrote:
Show 6 quoted lines
> This strcpy will never overflow because it's copying from
> baked-in test data. But we would prefer to avoid strcpy
> entirely, as it makes it harder to audit for real security
> bugs.
> 
> Signed-off-by: Jeff King <peff@peff.net>
ACK, of course.

Thanks! Dscho

← back to recent threads