{"thread":{"id":"21619","subject":"[PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions.","startedAt":"2009-11-14T21:10:57Z","lastAt":"2009-11-23T17:20:07Z","messageCount":8,"participants":["Tarmigan Casebolt","Shawn O. Pearce","Jeff King","Junio C Hamano","Tarmigan","Brian Gernhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"127561","messageId":"1258233058-2348-1-git-send-email-tarmigan+git@gmail.com","threadId":"21619","inReplyTo":null,"subject":"[PATCH 1/2] http-backend: Fix access beyond end of string.","fromName":"Tarmigan Casebolt","fromEmail":"tarmigan+git@gmail.com","sentAt":"2009-11-14T21:10:57Z","receivedAt":"2009-11-14T21:10:57Z","isPatch":true,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"Found with valgrind while looking for Content-Length corruption in\nsmart http.\n\nSigned-off-by: Tarmigan Casebolt \n---\n http-backend.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex f8ea9d7..ab9433d 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -634,7 +634,7 @@ int main(int argc, char **argv)\n \t\t\tcmd = c;\n \t\t\tcmd_arg = xmalloc(n);\n \t\t\tstrncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n-\t\t\tcmd_arg[n] = '\\0';\n+\t\t\tcmd_arg[n-1] = '\\0';\n \t\t\tdir[out[0].rm_so] = 0;\n \t\t\tbreak;\n \t\t}\n-- \n1.6.5.51.g191f5\n"},{"id":"127560","messageId":"1258233058-2348-2-git-send-email-tarmigan+git@gmail.com","threadId":"21619","inReplyTo":"1258233058-2348-1-git-send-email-tarmigan+git@gmail.com","subject":"[PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions.","fromName":"Tarmigan Casebolt","fromEmail":"tarmigan+git@gmail.com","sentAt":"2009-11-14T21:10:58Z","receivedAt":"2009-11-14T21:10:58Z","isPatch":true,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"We already have these checks in many printf-type functions that have\nprototypes which are in header files. Add these same checks to\nstatic functions in http-backend.c\n\nSigned-off-by: Tarmigan Casebolt \n---\n\nShawn, please consider this patch in addition to the one that you posted \nthat actually fixes the bug. With this patch, gcc will warn about that bug.\n\n http-backend.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ab9433d..110b166 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -108,6 +108,7 @@ static const char *get_parameter(const char *name)\n \treturn i ? i->util : NULL;\n }\n \n+__attribute__((format (printf, 2, 3)))\n static void format_write(int fd, const char *fmt, ...)\n {\n \tstatic char buffer[1024];\n@@ -165,6 +166,7 @@ static void end_headers(void)\n \tsafe_write(1, \"\\r\\n\", 2);\n }\n \n+__attribute__((format (printf, 1, 2)))\n static NORETURN void not_found(const char *err, ...)\n {\n \tva_list params;\n@@ -180,6 +182,7 @@ static NORETURN void not_found(const char *err, ...)\n \texit(0);\n }\n \n+__attribute__((format (printf, 1, 2)))\n static NORETURN void forbidden(const char *err, ...)\n {\n \tva_list params;\n-- \n1.6.5.51.g191f5\n"},{"id":"127653","messageId":"20091116013654.GX11919@spearce.org","threadId":"21619","inReplyTo":"1258233058-2348-1-git-send-email-tarmigan+git@gmail.com","subject":"Re: [PATCH 1/2] http-backend: Fix access beyond end of string.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-11-16T01:36:54Z","receivedAt":"2009-11-16T01:36:54Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Tarmigan Casebolt wrote:\n> diff --git a/http-backend.c b/http-backend.c\n> index f8ea9d7..ab9433d 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -634,7 +634,7 @@ int main(int argc, char **argv)\n> \t\t\tcmd = c;\n> \t\t\tcmd_arg = xmalloc(n);\n> \t\t\tstrncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n> -\t\t\tcmd_arg[n] = '\\0';\n> +\t\t\tcmd_arg[n-1] = '\\0';\n> \t\t\tdir[out[0].rm_so] = 0;\n> \t\t\tbreak;\n\nShouldn't this instead be:\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 9021266..16ec635 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -626,7 +626,7 @@ int main(int argc, char **argv)\n \t\t\t}\n \n \t\t\tcmd = c;\n-\t\t\tcmd_arg = xmalloc(n);\n+\t\t\tcmd_arg = xmalloc(n + 1);\n \t\t\tstrncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n \t\t\tcmd_arg[n] = '\\0';\n \t\t\tdir[out[0].rm_so] = 0;\n\nThe cmd_arg string was simply allocated too small. Your fix is\nterminating the string one character too short which would cause\nget_loose_object and get_pack_file to break.\n\n-- \nShawn.\n"},{"id":"127654","messageId":"20091116013911.GY11919@spearce.org","threadId":"21619","inReplyTo":"1258233058-2348-2-git-send-email-tarmigan+git@gmail.com","subject":"Re: [PATCH 2/2] http-backend: Let gcc check the format of more printf-type functions.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-11-16T01:39:11Z","receivedAt":"2009-11-16T01:39:11Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Tarmigan Casebolt wrote:\n> We already have these checks in many printf-type functions that have\n> prototypes which are in header files. Add these same checks to\n> static functions in http-backend.c\n> \n> Signed-off-by: Tarmigan Casebolt \n> ---\n> \n> Shawn, please consider this patch in addition to the one that you posted \n> that actually fixes the bug. With this patch, gcc will warn about that bug.\n\nYup, it would have caught it, thanks.\n\nAcked-by: Shawn O. Pearce \n \n-- \nShawn.\n"},{"id":"127658","messageId":"20091116045532.GC14664@coredump.intra.peff.net","threadId":"21619","inReplyTo":"20091116013654.GX11919@spearce.org","subject":"Re: [PATCH 1/2] http-backend: Fix access beyond end of string.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-16T04:55:32Z","receivedAt":"2009-11-16T04:55:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:\n\n> Tarmigan Casebolt wrote:\n> > diff --git a/http-backend.c b/http-backend.c\n> > index f8ea9d7..ab9433d 100644\n> > --- a/http-backend.c\n> > +++ b/http-backend.c\n> > @@ -634,7 +634,7 @@ int main(int argc, char **argv)\n> > \t\t\tcmd = c;\n> > \t\t\tcmd_arg = xmalloc(n);\n> > \t\t\tstrncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n> > -\t\t\tcmd_arg[n] = '\\0';\n> > +\t\t\tcmd_arg[n-1] = '\\0';\n> > \t\t\tdir[out[0].rm_so] = 0;\n> > \t\t\tbreak;\n> \n> Shouldn't this instead be:\n> \n> diff --git a/http-backend.c b/http-backend.c\n> index 9021266..16ec635 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -626,7 +626,7 @@ int main(int argc, char **argv)\n> \t\t\t}\n> \n> \t\t\tcmd = c;\n> -\t\t\tcmd_arg = xmalloc(n);\n> +\t\t\tcmd_arg = xmalloc(n + 1);\n> \t\t\tstrncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n> \t\t\tcmd_arg[n] = '\\0';\n> \t\t\tdir[out[0].rm_so] = 0;\n> \n> The cmd_arg string was simply allocated too small. Your fix is\n> terminating the string one character too short which would cause\n> get_loose_object and get_pack_file to break.\n\nActually, from my reading, I think his fix is right, because you trim\nthe first character during the strncpy (using \"out[0].rm_so + 1\"). But\nit's not clear when you create 'n' that you are dropping that character.\nIOW, you are doing:\n\n /* string + '\\0' - '/' */\n size_t n = out[0].rm_eo - (out[0].rm_so + 1) + 1;\n\nwhich ends up the same as your n, but means that the NUL goes at\ncmd_arg[n-1]. But I didn't actually run it, so if his fix is breaking\nthings, then both Tarmigan and I are counting wrong. ;)\n\n-Peff\n"},{"id":"127660","messageId":"7viqdb0zhs.fsf@alter.siamese.dyndns.org","threadId":"21619","inReplyTo":"20091116045532.GC14664@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] http-backend: Fix access beyond end of string.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-16T06:12:31Z","receivedAt":"2009-11-16T06:12:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:\n> ...\n>> Shouldn't this instead be:\n>> \n>> diff --git a/http-backend.c b/http-backend.c\n>> index 9021266..16ec635 100644\n>> --- a/http-backend.c\n>> +++ b/http-backend.c\n>> @@ -626,7 +626,7 @@ int main(int argc, char **argv)\n>> \t\t\t}\n>> \n>> \t\t\tcmd = c;\n>> -\t\t\tcmd_arg = xmalloc(n);\n>> +\t\t\tcmd_arg = xmalloc(n + 1);\n>> \t\t\tstrncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n>> \t\t\tcmd_arg[n] = '\\0';\n>> \t\t\tdir[out[0].rm_so] = 0;\n>> \n>> The cmd_arg string was simply allocated too small. Your fix is\n>> terminating the string one character too short which would cause\n>> get_loose_object and get_pack_file to break.\n>\n> Actually, from my reading, I think his fix is right, because you trim\n> the first character during the strncpy (using \"out[0].rm_so + 1\").\n\nYour regexps all start with leading \"/\", and rm_so+1 points at the\ncharacter after the slash; the intention being that you would copy\nthe rest of the matched sequence without the leading \"/\".\n\nSo allocating n = rm_eo - rm_so is Ok. It counts the space for\nterminating NUL. But copying \"up to n bytes\" using strncpy(), only to NUL\nterminate immediately later, is dubious. You would want to copy only n-1\nbytes. I.e.\n\n\tn = out[0].rm_eo - out[0].rm_so; /* allocation */\n ... validate and fail invalid method ...\n cmd_arg = xmalloc(n);\n memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1);\n cmd_arg[n-1] = '\\0';\n"},{"id":"127730","messageId":"905315640911162046q219c07d9w425f6bde0e1ba870@mail.gmail.com","threadId":"21619","inReplyTo":"7viqdb0zhs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] http-backend: Fix access beyond end of string.","fromName":"Tarmigan","fromEmail":"tarmigan+git@gmail.com","sentAt":"2009-11-17T04:46:38Z","receivedAt":"2009-11-17T04:46:38Z","isPatch":true,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"On Sun, Nov 15, 2009 at 10:12 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Sun, Nov 15, 2009 at 05:36:54PM -0800, Shawn O. Pearce wrote:\n>> ...\n>>> Shouldn't this instead be:\n>>>\n>>> diff --git a/http-backend.c b/http-backend.c\n>>> index 9021266..16ec635 100644\n>>> --- a/http-backend.c\n>>> +++ b/http-backend.c\n>>> @@ -626,7 +626,7 @@ int main(int argc, char **argv)\n>>>                      }\n>>>\n>>>                      cmd = c;\n>>> -                    cmd_arg = xmalloc(n);\n>>> +                    cmd_arg = xmalloc(n + 1);\n>>>                      strncpy(cmd_arg, dir + out[0].rm_so + 1, n);\n>>>                      cmd_arg[n] = '\\0';\n>>>                      dir[out[0].rm_so] = 0;\n>>>\n>>> The cmd_arg string was simply allocated too small.  Your fix is\n>>> terminating the string one character too short which would cause\n>>> get_loose_object and get_pack_file to break.\n>>\n>> Actually, from my reading, I think his fix is right, because you trim\n>> the first character during the strncpy (using \"out[0].rm_so + 1\").\n>\n> Your regexps all start with leading \"/\", and rm_so+1 points at the\n> character after the slash; the intention being that you would copy\n> the rest of the matched sequence without the leading \"/\".\n>\n> So allocating n = rm_eo - rm_so is Ok.  It counts the space for\n> terminating NUL.  But copying \"up to n bytes\" using strncpy(), only to NUL\n> terminate immediately later, is dubious.  You would want to copy only n-1\n> bytes.  I.e.\n>\n>        n = out[0].rm_eo - out[0].rm_so; /* allocation */\n>        ... validate and fail invalid method ...\n>        cmd_arg = xmalloc(n);\n>        memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1);\n>        cmd_arg[n-1] = '\\0';\n>\n\nI think the strncpy( , ,n) would not harm anything because we won't\noverflow dir because it's NUL terminated in getdir(), and the '\\0'\nshouldn't match the regex. But I agree that strncpy( , , n-1) is\nbetter and memcpy( , , n-1) is better still.\n\nBetter eyes than mine have now looked at this and see different things\neach time. I wonder if some parts could be made a little less subtle\n(perhaps along with the dir[out[0].rm_so] = 0;)?\n\nThanks,\nTarmigan\n"},{"id":"128180","messageId":"9201C178-AABF-4320-B7B0-FEE841300E69@gernhardtsoftware.com","threadId":"21619","inReplyTo":"7viqdb0zhs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] http-backend: Fix access beyond end of string.","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2009-11-23T17:20:07Z","receivedAt":"2009-11-23T17:20:07Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"\nOn Nov 16, 2009, at 1:12 AM, Junio C Hamano wrote:\n\n> \tn = out[0].rm_eo - out[0].rm_so; /* allocation */\n> ... validate and fail invalid method ...\n> cmd_arg = xmalloc(n);\n> memcpy(cmd_arg, dir + out[0].rm_so + 1, n-1);\n> cmd_arg[n-1] = '\\0';\n\nI just thought I'd point out that this change (committed as 48aec1b) fixed the problem I was having with t5541-http-push (and a couple others) hanging. Looks like that one extra byte was overwriting something that malloc/free wanted to keep intact on OS X.\n\n~~ Brian"}]}