{"thread":{"id":"6970","subject":"[PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","startedAt":"2007-02-26T12:08:09Z","lastAt":"2007-02-27T19:35:08Z","messageCount":10,"participants":["Johannes Schindelin","Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"35499","messageId":"Pine.LNX.4.63.0702261306140.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6970","inReplyTo":null,"subject":"[PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T12:08:09Z","receivedAt":"2007-02-26T12:08:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n---\n builtin-fetch--tool.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex 48de08d..e5bb560 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -14,6 +14,10 @@ static char *get_stdin(void)\n \t\tdata = xrealloc(data, offset + CHUNK_SIZE);\n \t\tread = xread(0, data + offset, CHUNK_SIZE);\n \t}\n+\tif (read > 0 && data[read - 1] == '\\n')\n+\t\tdata[read - 1] = '\\0';\n+\telse\n+\t\tdata[read] = '\\0';\n \treturn data;\n }\n \n-- \n1.5.0.1.2424.g39563\n"},{"id":"35505","messageId":"Pine.LNX.4.64.0702260821310.12485@woody.linux-foundation.org","threadId":"6970","inReplyTo":"Pine.LNX.4.63.0702261306140.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T16:24:35Z","receivedAt":"2007-02-26T16:24:35Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 26 Feb 2007, Johannes Schindelin wrote:\n> \n> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> ---\n>  builtin-fetch--tool.c |    4 ++++\n>  1 files changed, 4 insertions(+), 0 deletions(-)\n> \n> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\n> index 48de08d..e5bb560 100644\n> --- a/builtin-fetch--tool.c\n> +++ b/builtin-fetch--tool.c\n> @@ -14,6 +14,10 @@ static char *get_stdin(void)\n>  \t\tdata = xrealloc(data, offset + CHUNK_SIZE);\n>  \t\tread = xread(0, data + offset, CHUNK_SIZE);\n>  \t}\n> +\tif (read > 0 && data[read - 1] == '\\n')\n> +\t\tdata[read - 1] = '\\0';\n> +\telse\n> +\t\tdata[read] = '\\0';\n>  \treturn data;\n>  }\n\nThis is horrible crap.\n\nFirst off, \"read\" here may be -1.\n\nSecondly, \"data[read]\", even if read is positive, is in the *middle* of a \nbuffer.\n\nIt should probably be something like\n\n\tif (read > 0)\n\t\toffset += read;\n\n\t/* Why do this? Because Dscho did.. */\n\tif (offset && data[offset-1] == '\\n')\n\t\toffset--;\n\n\tdata[offset] = 0;\n\nwhich at least seems to be potentially sane.\n\n\t\tLinus\n"},{"id":"35509","messageId":"Pine.LNX.4.63.0702261741360.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6970","inReplyTo":"Pine.LNX.4.64.0702260821310.12485@woody.linux-foundation.org","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T16:47:31Z","receivedAt":"2007-02-26T16:47:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 26 Feb 2007, Linus Torvalds wrote:\n\n> > +\tif (read > 0 && data[read - 1] == '\\n')\n> > +\t\tdata[read - 1] = '\\0';\n> > +\telse\n> > +\t\tdata[read] = '\\0';\n> \n> First off, \"read\" here may be -1.\n\nAh yes. I somehow had the erroneous impression that xread() die()s on \nerror like xmalloc()...\n\n> \t/* Why do this? Because Dscho did.. */\n> \tif (offset && data[offset-1] == '\\n')\n> \t\toffset--;\n\nWell yes, it probably does not change anything except the output, which \nyou see only when debugging.\n\nIt's not like I care deeply, because this part of the code will hopefully \nsoon go away, when git-fetch is a proper builtin.\n\nHowever, I compile with -DXMALLOC_POISON=1 and _that_ did not play well, \nwhereas the read fails rather rarely on stdin...\n\nBut I agree your patch is saner.\n\nCiao,\nDscho\n"},{"id":"35511","messageId":"Pine.LNX.4.64.0702260905420.12485@woody.linux-foundation.org","threadId":"6970","inReplyTo":"Pine.LNX.4.63.0702261741360.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T17:06:36Z","receivedAt":"2007-02-26T17:06:36Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 26 Feb 2007, Johannes Schindelin wrote:\n> \n> But I agree your patch is saner.\n\nWell, it's not the \"saner\" I'm worried about, I was more worried about  \nthe fact that your patch added random '\\0' characters in the middle of the \nstdin input if it was ever bigger than CHUNK_SIZE. I suspect you only \ntested with small input to stdin.\n\n\t\t\tLinus\n"},{"id":"35513","messageId":"Pine.LNX.4.63.0702261827050.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6970","inReplyTo":"Pine.LNX.4.64.0702260905420.12485@woody.linux-foundation.org","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T17:27:27Z","receivedAt":"2007-02-26T17:27:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 26 Feb 2007, Linus Torvalds wrote:\n\n> On Mon, 26 Feb 2007, Johannes Schindelin wrote:\n> > \n> > But I agree your patch is saner.\n> \n> Well, it's not the \"saner\" I'm worried about, I was more worried about \n> the fact that your patch added random '\\0' characters in the middle of \n> the stdin input if it was ever bigger than CHUNK_SIZE. I suspect you \n> only tested with small input to stdin.\n\nOoops. I did not even realize that after reading your reply.\n\nCiao,\nDscho\n"},{"id":"35514","messageId":"Pine.LNX.4.63.0702261827510.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6970","inReplyTo":"Pine.LNX.4.64.0702260905420.12485@woody.linux-foundation.org","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T17:28:35Z","receivedAt":"2007-02-26T17:28:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 26 Feb 2007, Linus Torvalds wrote:\n\n> I suspect you only tested with small input to stdin.\n\nAddendum: yes, I only tested with a reflist which was smaller than one \nmegabyte ;-)\n\nCiao,\nDscho\n"},{"id":"35518","messageId":"Pine.LNX.4.64.0702261003480.12485@woody.linux-foundation.org","threadId":"6970","inReplyTo":"Pine.LNX.4.63.0702261827510.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T18:05:59Z","receivedAt":"2007-02-26T18:05:59Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 26 Feb 2007, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Mon, 26 Feb 2007, Linus Torvalds wrote:\n> \n> > I suspect you only tested with small input to stdin.\n> \n> Addendum: yes, I only tested with a reflist which was smaller than one \n> megabyte ;-)\n\nWell, that CHUNK_SIZE is just silly. I don't see why you'd have a \nchunk-size of a megabyte to begin with, IO doesn't really get any more \nefficient that way. And yeah, in this case it would easily hide the bug, \nbecause in practice nobody would ever test with that much input data.\n\nIt might make sense to make the chunk-size smaller just from a testability \nstandpoint (not to mention that it's probably currently just wasting \nmemory for most users - although at least under Linux, if you never use a \npage, none will be allocated for you, so the OS may hide the wastage).\n\n\t\tLinus\n"},{"id":"35525","messageId":"7vk5y4n2qw.fsf@assigned-by-dhcp.cox.net","threadId":"6970","inReplyTo":"Pine.LNX.4.64.0702261003480.12485@woody.linux-foundation.org","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-26T19:21:59Z","receivedAt":"2007-02-26T19:21:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Mon, 26 Feb 2007, Johannes Schindelin wrote:\n>\n>> Hi,\n>> \n>> On Mon, 26 Feb 2007, Linus Torvalds wrote:\n>> \n>> > I suspect you only tested with small input to stdin.\n>> \n>> Addendum: yes, I only tested with a reflist which was smaller than one \n>> megabyte ;-)\n>\n> Well, that CHUNK_SIZE is just silly. I don't see why you'd have a \n> chunk-size of a megabyte to begin with,...\n\nMe neither (not my code) ;-).  I agree with you that using\nsmaller limit than what will be deployed often helps\ntestability.\n"},{"id":"35529","messageId":"7vfy8sn20o.fsf@assigned-by-dhcp.cox.net","threadId":"6970","inReplyTo":"Pine.LNX.4.64.0702261003480.12485@woody.linux-foundation.org","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-26T19:37:43Z","receivedAt":"2007-02-26T19:37:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Well, that CHUNK_SIZE is just silly. I don't see why you'd have a \n> chunk-size of a megabyte to begin with, IO doesn't really get any more \n> efficient that way. And yeah, in this case it would easily hide the bug, \n> because in practice nobody would ever test with that much input data.\n>\n> It might make sense to make the chunk-size smaller just from a testability \n> standpoint (not to mention that it's probably currently just wasting \n> memory for most users - although at least under Linux, if you never use a \n> page, none will be allocated for you, so the OS may hide the wastage).\n\nHow about doing this instead?\n\n-- >8 --\n[PATCH] fetch--tool: fix uninitialized buffer when reading from stdin\n\nThe original code allocates too much space and forgets to NUL\nterminate the string.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n builtin-fetch--tool.c |   19 +++++++++++++------\n 1 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex 48de08d..a068f8d 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -2,17 +2,24 @@\n #include \"refs.h\"\n #include \"commit.h\"\n \n-#define CHUNK_SIZE (1048576)\n+#define CHUNK_SIZE 1024\n \n static char *get_stdin(void)\n {\n+\tint offset = 0;\n \tchar *data = xmalloc(CHUNK_SIZE);\n-\tint offset = 0, read = 0;\n-\tread = xread(0, data, CHUNK_SIZE);\n-\twhile (read == CHUNK_SIZE) {\n-\t\toffset += CHUNK_SIZE;\n+\n+\twhile (1) {\n+\t\tint cnt = xread(0, data + offset, CHUNK_SIZE);\n+\t\tif (cnt < 0)\n+\t\t\tdie(\"error reading standard input: %s\",\n+\t\t\t    strerror(errno));\n+\t\tif (cnt == 0) {\n+\t\t\tdata[offset] = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t\toffset += cnt;\n \t\tdata = xrealloc(data, offset + CHUNK_SIZE);\n-\t\tread = xread(0, data + offset, CHUNK_SIZE);\n \t}\n \treturn data;\n }\n"},{"id":"35653","messageId":"Pine.LNX.4.63.0702272034010.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6970","inReplyTo":"7vfy8sn20o.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-27T19:35:08Z","receivedAt":"2007-02-27T19:35:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi\n\nOn Mon, 26 Feb 2007, Junio C Hamano wrote:\n\n> [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin\n> \n> The original code allocates too much space and forgets to NUL\n> terminate the string.\n> \n> Signed-off-by: Junio C Hamano <junkio@cox.net>\n> ---\n> \n>  builtin-fetch--tool.c |   19 +++++++++++++------\n>  1 files changed, 13 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\n> index 48de08d..a068f8d 100644\n> --- a/builtin-fetch--tool.c\n> +++ b/builtin-fetch--tool.c\n> @@ -2,17 +2,24 @@\n>  #include \"refs.h\"\n>  #include \"commit.h\"\n>  \n> -#define CHUNK_SIZE (1048576)\n> +#define CHUNK_SIZE 1024\n>  \n>  static char *get_stdin(void)\n>  {\n> +\tint offset = 0;\n>  \tchar *data = xmalloc(CHUNK_SIZE);\n> -\tint offset = 0, read = 0;\n> -\tread = xread(0, data, CHUNK_SIZE);\n> -\twhile (read == CHUNK_SIZE) {\n> -\t\toffset += CHUNK_SIZE;\n> +\n> +\twhile (1) {\n> +\t\tint cnt = xread(0, data + offset, CHUNK_SIZE);\n> +\t\tif (cnt < 0)\n> +\t\t\tdie(\"error reading standard input: %s\",\n> +\t\t\t    strerror(errno));\n> +\t\tif (cnt == 0) {\n> +\t\t\tdata[offset] = 0;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\toffset += cnt;\n>  \t\tdata = xrealloc(data, offset + CHUNK_SIZE);\n> -\t\tread = xread(0, data + offset, CHUNK_SIZE);\n>  \t}\n>  \treturn data;\n>  }\n\nMe-liked-by: Dscho. And maybe people forget about my silly \"fix\"...\n\nCiao,\nDscho\n"}]}