# [PATCH] test-path-utils: use xsnprintf in favor of strcpy 7 messages from 2016-02-08 to 2016-02-09. Participants: Jeff King, Junio C Hamano, Eric Wong, Johannes Schindelin. Thread: https://gitlist.dev/t/41373 ## Jeff King, 2016-02-08 22:21 Subject: [PATCH] test-path-utils: use xsnprintf in favor of strcpy Message-ID: <20160208222155.GA17395@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160208222155.GA17395%40sigill.intra.peff.net ``` 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 --- 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 ``` ## Jeff King, 2016-02-08 22:25 Subject: [PATCH] rerere: replace strcpy with xsnprintf Message-ID: <20160208222501.GA21671@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160208222501.GA21671%40sigill.intra.peff.net In-Reply-To: <20160208222155.GA17395@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Junio C Hamano, 2016-02-08 22:41 Subject: Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy Message-ID: URL: https://gitlist.dev/e/xmqqh9hi6d00.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160208222155.GA17395@sigill.intra.peff.net> ``` Jeff King 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 > --- > 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)) ``` ## Eric Wong, 2016-02-08 23:07 Subject: Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy Message-ID: <20160208230726.GB3487@dcvr.yhbt.net> URL: https://gitlist.dev/e/20160208230726.GB3487%40dcvr.yhbt.net In-Reply-To: <20160208222155.GA17395@sigill.intra.peff.net> ``` Jeff King 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... ``` ## Junio C Hamano, 2016-02-08 23:07 Subject: Re: [PATCH] rerere: replace strcpy with xsnprintf Message-ID: URL: https://gitlist.dev/e/xmqqd1s66bt8.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160208222501.GA21671@sigill.intra.peff.net> ``` Jeff King 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 > --- > 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; > } ``` ## Jeff King, 2016-02-08 23:13 Subject: Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy Message-ID: <20160208231332.GA23638@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160208231332.GA23638%40sigill.intra.peff.net In-Reply-To: <20160208230726.GB3487@dcvr.yhbt.net> ``` On Mon, Feb 08, 2016 at 11:07:26PM +0000, Eric Wong wrote: > Jeff King 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, 2016-02-09 10:13 Subject: Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1602091112570.2964%40virtualbox In-Reply-To: <20160208222155.GA17395@sigill.intra.peff.net> ``` 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 ACK, of course. Thanks! Dscho ```