{"thread":{"id":"7039","subject":"Another memory overrun in http-push.c","startedAt":"2007-03-01T16:09:12Z","lastAt":"2007-03-02T10:03:57Z","messageCount":3,"participants":["Eygene Ryabinkin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"35982","messageId":"20070301160911.GU57456@codelabs.ru","threadId":"7039","inReplyTo":null,"subject":"Another memory overrun in http-push.c","fromName":"Eygene Ryabinkin","fromEmail":"rea-git@codelabs.ru","sentAt":"2007-03-01T16:09:12Z","receivedAt":"2007-03-01T16:09:12Z","isPatch":false,"sender":{"key":"rea-git@codelabs.ru","avatar":null},"body":"Me again ;))\n\nSpotted another memory overrun in the http-push.c. In principle,\nit is the read-only overrun, but it provokes the coredump on my\nsystem.  The problem is that strlcpy(dst, src, size) returns the\nlength of the 'src' and demands it to be NULL-terminated (see\n'man strlcpy' and http://www.gratisoft.us/todd/papers/strlcpy.html).\nIt is not the case for the xml_cdata and possibly other places. So\nI've just replaced strlcpy with memcpy + zero termination all over\nthe http-push.c. The patch is below.\n\n--- http-push.c.orig\tThu Mar  1 18:48:19 2007\n+++ http-push.c\tThu Mar  1 18:55:24 2007\n@@ -1271,7 +1271,9 @@\n \tstruct xml_ctx *ctx = (struct xml_ctx *)userData;\n \tfree(ctx->cdata);\n \tctx->cdata = xmalloc(len + 1);\n-\tstrlcpy(ctx->cdata, s, len + 1);\n+\t/* NB: 's' is not null-terminated, can not use strlcpy here */\n+\tmemcpy(ctx->cdata, s, len);\n+\tctx->cdata[len] = '\\0';\n }\n \n static struct remote_lock *lock_remote(const char *path, long timeout)\n@@ -1473,7 +1475,8 @@\n \t\treturn;\n \tpath += 8;\n \tobj_hex = xmalloc(strlen(path));\n-\tstrlcpy(obj_hex, path, 3);\n+\t/* NB: path is not null-terminated, can not use strlcpy here */\n+\tmemcpy(obj_hex, path, 2);\n \tstrcpy(obj_hex + 2, path + 3);\n \tone_remote_object(obj_hex);\n \tfree(obj_hex);\n@@ -2170,7 +2173,8 @@\n \t/* If it's a symref, set the refname; otherwise try for a sha1 */\n \tif (!strncmp((char *)buffer.buffer, \"ref: \", 5)) {\n \t\t*symref = xmalloc(buffer.posn - 5);\n-\t\tstrlcpy(*symref, (char *)buffer.buffer + 5, buffer.posn - 5);\n+\t\tmemcpy(*symref, (char *)buffer.buffer + 5, buffer.posn - 6);\n+\t\t(*symref)[buffer.posn - 6] = '\\0';\n \t} else {\n \t\tget_sha1_hex(buffer.buffer, sha1);\n \t}\n\nmemcpy(obj_hex, path, 2) is not followed by zero-termination since\nit will be done by the strcpy that is following.\n\nThis cured my git-http-push and let it do all PROPFINDS on the rather\nlarge repository (175 Mb). Now I have only one SEGV that is happening\ninside the libcurl both in http-push.c and http-fetch.c. Already\ntalking to CURL people and trying to write the clear testcase for\nthe problem.\n-- \nEygene\n"},{"id":"36046","messageId":"7vwt20giw3.fsf@assigned-by-dhcp.cox.net","threadId":"7039","inReplyTo":"20070301160911.GU57456@codelabs.ru","subject":"Re: Another memory overrun in http-push.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-02T08:16:28Z","receivedAt":"2007-03-02T08:16:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eygene Ryabinkin <rea-git@codelabs.ru> writes:\n\n> Me again ;))\n>\n> Spotted another memory overrun in the http-push.c. In principle,\n> it is the read-only overrun, but it provokes the coredump on my\n> system.  The problem is that strlcpy(dst, src, size) returns the\n> length of the 'src' and demands it to be NULL-terminated (see\n> 'man strlcpy' and http://www.gratisoft.us/todd/papers/strlcpy.html).\n> It is not the case for the xml_cdata and possibly other places. So\n> I've just replaced strlcpy with memcpy + zero termination all over\n> the http-push.c. The patch is below.\n\nPlease check Documentation/SubmittingPatches.\n\nUse of strlcpy() in general _is_ stupid if you are computing how\nmuch space is needed, allocating that much as your own buffer\nand then copying.  strlcpy() needs to say how much it would have\ncopied if it were given large enough buffer, and it needs to be\nable to run strlen(src), so it is not valid to give a buffer\nthat may not be NUL-terminated as you say.\n"},{"id":"36059","messageId":"20070302100356.GE57456@codelabs.ru","threadId":"7039","inReplyTo":"7vwt20giw3.fsf@assigned-by-dhcp.cox.net","subject":"Re: Another memory overrun in http-push.c","fromName":"Eygene Ryabinkin","fromEmail":"rea-git@codelabs.ru","sentAt":"2007-03-02T10:03:57Z","receivedAt":"2007-03-02T10:03:57Z","isPatch":false,"sender":{"key":"rea-git@codelabs.ru","avatar":null},"body":"Junio, good day!\n\n> > Spotted another memory overrun in the http-push.c. In principle,\n> > it is the read-only overrun, but it provokes the coredump on my\n> > system.  The problem is that strlcpy(dst, src, size) returns the\n> > length of the 'src' and demands it to be NULL-terminated (see\n> > 'man strlcpy' and http://www.gratisoft.us/todd/papers/strlcpy.html).\n> > It is not the case for the xml_cdata and possibly other places. So\n> > I've just replaced strlcpy with memcpy + zero termination all over\n> > the http-push.c. The patch is below.\n> \n> Please check Documentation/SubmittingPatches.\n\nThanks, just read and enlightened.\n\n> Use of strlcpy() in general _is_ stupid if you are computing how\n> much space is needed, allocating that much as your own buffer\n> and then copying.  strlcpy() needs to say how much it would have\n> copied if it were given large enough buffer, and it needs to be\n> able to run strlen(src), so it is not valid to give a buffer\n> that may not be NUL-terminated as you say.\n\nYes, you're perfectly right: the strncpy, bcopy or memcpy + zero-termination\nare preferrable in this situation.\n-- \nEygene\n"}]}