{"thread":{"id":"41194","subject":"[PATCH] test-path-utils: use xsnprintf in favor of strcpy","startedAt":"2016-01-14T20:26:08Z","lastAt":"2016-01-19T11:05:55Z","messageCount":4,"participants":["Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"276075","messageId":"20160114202608.GA8806@sigill.intra.peff.net","threadId":"41194","inReplyTo":null,"subject":"[PATCH] test-path-utils: use xsnprintf in favor of strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-14T20:26:08Z","receivedAt":"2016-01-14T20:26:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This strcpy will never overflow because it's copying from\nbaked-in test data. But we would prefer to avoid strcpy\nentirely, as it makes it harder to audit for real security\nbugs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI admit that an audit could probably just avoid looking at test-* in the\nfirst place, but not all do (coverity complained about this one, for\nexample).\n\nThis sort-of applies on top of js/dirname-basename, which is in next.\nTextually, it's fine, but that topic is based on v2.6.5, and xsnprintf\nwas only added in the v2.7.0 cycle. The simplest thing is probably to\nwait for it to graduate to master, and then apply there as a new topic\n(if we do v2.6.6, it's OK for it not to have this patch).\n\nI can hold and resend in a week or two if that's easier.\n\n test-path-utils.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/test-path-utils.c b/test-path-utils.c\nindex 4ab68ac..b9ece10 100644\n--- a/test-path-utils.c\n+++ b/test-path-utils.c\n@@ -55,7 +55,7 @@ static int test_function(struct test_data *data, char *(*func)(char *input),\n \t\tif (!data[i].from)\n \t\t\tto = func(NULL);\n \t\telse {\n-\t\t\tstrcpy(buffer, data[i].from);\n+\t\t\txsnprintf(buffer, sizeof(buffer), \"%s\", data[i].from);\n \t\t\tto = func(buffer);\n \t\t}\n \t\tif (strcmp(to, data[i].to)) {\n-- \n2.7.0.244.g0701a9d\n"},{"id":"276125","messageId":"alpine.DEB.2.20.1601150743590.2964@virtualbox","threadId":"41194","inReplyTo":"20160114202608.GA8806@sigill.intra.peff.net","subject":"Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-15T06:45:16Z","receivedAt":"2016-01-15T06:45:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 14 Jan 2016, Jeff King wrote:\n\n> This strcpy will never overflow because it's copying from\n> baked-in test data. But we would prefer to avoid strcpy\n> entirely, as it makes it harder to audit for real security\n> bugs.\n\nThanks.\n\n> This sort-of applies on top of js/dirname-basename, which is in next.\n> Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf\n> was only added in the v2.7.0 cycle. The simplest thing is probably to\n> wait for it to graduate to master, and then apply there as a new topic\n> (if we do v2.6.6, it's OK for it not to have this patch).\n> \n> I can hold and resend in a week or two if that's easier.\n\nIf you have a patch to make dirname/basename safer based on xsnprintf, I\nwould like to have that as soon as possible (next was rewound to 2.7.0,\nno?)...\n\nThanks!\nDscho\n"},{"id":"276182","messageId":"20160115183010.GA10431@sigill.intra.peff.net","threadId":"41194","inReplyTo":"alpine.DEB.2.20.1601150743590.2964@virtualbox","subject":"Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-15T18:30:10Z","receivedAt":"2016-01-15T18:30:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 15, 2016 at 07:45:16AM +0100, Johannes Schindelin wrote:\n\n> > This sort-of applies on top of js/dirname-basename, which is in next.\n> > Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf\n> > was only added in the v2.7.0 cycle. The simplest thing is probably to\n> > wait for it to graduate to master, and then apply there as a new topic\n> > (if we do v2.6.6, it's OK for it not to have this patch).\n> > \n> > I can hold and resend in a week or two if that's easier.\n> \n> If you have a patch to make dirname/basename safer based on xsnprintf, I\n> would like to have that as soon as possible (next was rewound to 2.7.0,\n> no?)...\n\nI'm not sure what you mean. `dirname/basename` themselves don't have any\nproblems. It's only the `strcpy` in the test program that I wanted to\nfix.\n\nIf Junio wants to rebase js/dirname-basename on a more recent tip (say,\ncurrent \"master\") as part of the rewind, this could be applied directly\n(or just squashed in).\n\n-Peff\n"},{"id":"276340","messageId":"alpine.DEB.2.20.1601191041260.2964@virtualbox","threadId":"41194","inReplyTo":"20160115183010.GA10431@sigill.intra.peff.net","subject":"Re: [PATCH] test-path-utils: use xsnprintf in favor of strcpy","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-19T11:05:55Z","receivedAt":"2016-01-19T11:05:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 15 Jan 2016, Jeff King wrote:\n\n> On Fri, Jan 15, 2016 at 07:45:16AM +0100, Johannes Schindelin wrote:\n> \n> > > This sort-of applies on top of js/dirname-basename, which is in next.\n> > > Textually, it's fine, but that topic is based on v2.6.5, and xsnprintf\n> > > was only added in the v2.7.0 cycle. The simplest thing is probably to\n> > > wait for it to graduate to master, and then apply there as a new topic\n> > > (if we do v2.6.6, it's OK for it not to have this patch).\n> > > \n> > > I can hold and resend in a week or two if that's easier.\n> > \n> > If you have a patch to make dirname/basename safer based on xsnprintf, I\n> > would like to have that as soon as possible (next was rewound to 2.7.0,\n> > no?)...\n> \n> I'm not sure what you mean. `dirname/basename` themselves don't have any\n> problems. It's only the `strcpy` in the test program that I wanted to\n> fix.\n> \n> If Junio wants to rebase js/dirname-basename on a more recent tip (say,\n> current \"master\") as part of the rewind, this could be applied directly\n> (or just squashed in).\n\nMy bad... I misunderstood which code was affected.\n\nSorry for the noise,\nDscho\n"}]}