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

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

[Jeff King]

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(-)

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

[PATCH] rerere: replace strcpy with xsnprintf

[Jeff King]

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(-)

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

Re: [PATCH] rerere: replace strcpy with xsnprintf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

>
>  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;
>  }

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

>
>  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))

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

[Eric Wong]

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...

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

[Jeff King]

On Mon, Feb 08, 2016 at 11:07:26PM +0000, Eric Wong wrote:

> 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

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

[Johannes Schindelin]

Hi Peff,

On Mon, 8 Feb 2016, Jeff King wrote:

> 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

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

[Jeff King]

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>
---
I admit that an audit could probably just avoid looking at test-* in the
first place, but not all do (coverity complained about this one, for
example).

This sort-of applies on top of js/dirname-basename, which is in next.
Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf
was only added in the v2.7.0 cycle. The simplest thing is probably to
wait for it to graduate to master, and then apply there as a new topic
(if we do v2.6.6, it's OK for it not to have this patch).

I can hold and resend in a week or two if that's easier.

 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 4ab68ac..b9ece10 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -55,7 +55,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.0.244.g0701a9d

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

[Johannes Schindelin]

Hi Peff,

On Thu, 14 Jan 2016, Jeff King wrote:

> 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.

Thanks.

> This sort-of applies on top of js/dirname-basename, which is in next.
> Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf
> was only added in the v2.7.0 cycle. The simplest thing is probably to
> wait for it to graduate to master, and then apply there as a new topic
> (if we do v2.6.6, it's OK for it not to have this patch).
> 
> I can hold and resend in a week or two if that's easier.

If you have a patch to make dirname/basename safer based on xsnprintf, I
would like to have that as soon as possible (next was rewound to 2.7.0,
no?)...

Thanks!
Dscho

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

[Jeff King]

On Fri, Jan 15, 2016 at 07:45:16AM +0100, Johannes Schindelin wrote:

> > This sort-of applies on top of js/dirname-basename, which is in next.
> > Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf
> > was only added in the v2.7.0 cycle. The simplest thing is probably to
> > wait for it to graduate to master, and then apply there as a new topic
> > (if we do v2.6.6, it's OK for it not to have this patch).
> > 
> > I can hold and resend in a week or two if that's easier.
> 
> If you have a patch to make dirname/basename safer based on xsnprintf, I
> would like to have that as soon as possible (next was rewound to 2.7.0,
> no?)...

I'm not sure what you mean. `dirname/basename` themselves don't have any
problems. It's only the `strcpy` in the test program that I wanted to
fix.

If Junio wants to rebase js/dirname-basename on a more recent tip (say,
current "master") as part of the rewind, this could be applied directly
(or just squashed in).

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Fri, 15 Jan 2016, Jeff King wrote:

> On Fri, Jan 15, 2016 at 07:45:16AM +0100, Johannes Schindelin wrote:
> 
> > > This sort-of applies on top of js/dirname-basename, which is in next.
> > > Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf
> > > was only added in the v2.7.0 cycle. The simplest thing is probably to
> > > wait for it to graduate to master, and then apply there as a new topic
> > > (if we do v2.6.6, it's OK for it not to have this patch).
> > > 
> > > I can hold and resend in a week or two if that's easier.
> > 
> > If you have a patch to make dirname/basename safer based on xsnprintf, I
> > would like to have that as soon as possible (next was rewound to 2.7.0,
> > no?)...
> 
> I'm not sure what you mean. `dirname/basename` themselves don't have any
> problems. It's only the `strcpy` in the test program that I wanted to
> fix.
> 
> If Junio wants to rebase js/dirname-basename on a more recent tip (say,
> current "master") as part of the rewind, this could be applied directly
> (or just squashed in).

My bad... I misunderstood which code was affected.

Sorry for the noise,
Dscho