{"thread":{"id":"6975","subject":"Increase some limits in git-mailinfo","startedAt":"2007-02-26T18:43:03Z","lastAt":"2007-02-26T21:21:19Z","messageCount":8,"participants":["Linus Torvalds","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"35519","messageId":"Pine.LNX.4.64.0702261036550.12485@woody.linux-foundation.org","threadId":"6975","inReplyTo":null,"subject":"Increase some limits in git-mailinfo","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T18:43:03Z","receivedAt":"2007-02-26T18:43:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nI'm not proud of this patch, and I think we should do this better, but I \njust had trouble with an email that Eric Biederman sent me for the kernel, \nand this is the simplest and most obvious fix..\n\nWhat happened is that Eric's email had a long and complex \"References:\" \nline in his email headers, and the way git-mailinfo parses email headers, \nit will combine these multi-line headers into one single long line (which \nis appropriate for things like Subject lines etc, where everybody really \nwants to just have one long subject rather than know about rfc822 email \nmessage rules).\n\nHowever, with the extensive references, the end result was longer than the \none thousand character line limit we had, and what git-mailinfo did was to \njust stop parsing header lines. The end result: it never parsed the \n\"Date:\" line at all, and it left all subsequent email header lines as \n\"commit info\" contents instead. Very ugly.\n\nArguably that is the real bug (we should continue with header parsing), \nbut the trivial work-around is to just make the line buffer bigger.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nI'll look at making the parsing more robust too, but thought I'd send this \nout asap.\n\n\t\tLinus\n\n---\n builtin-mailinfo.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 6ee6b0b..3f0e08a 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -10,7 +10,7 @@ static FILE *cmitmsg, *patchfile, *fin, *fout;\n \n static int keep_subject;\n static const char *metainfo_charset;\n-static char line[1000];\n+static char line[10000];\n static char date[1000];\n static char name[1000];\n static char email[1000];\n"},{"id":"35524","messageId":"Pine.LNX.4.64.0702261107060.12485@woody.linux-foundation.org","threadId":"6975","inReplyTo":"Pine.LNX.4.64.0702261036550.12485@woody.linux-foundation.org","subject":"Re: Increase some limits in git-mailinfo","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T19:10:59Z","receivedAt":"2007-02-26T19:10:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 26 Feb 2007, Linus Torvalds wrote:\n> \n> I'm not proud of this patch, and I think we should do this better, but I \n> just had trouble with an email that Eric Biederman sent me for the kernel, \n> and this is the simplest and most obvious fix..\n\nOk, this is the more complicated fix for the problem.\n\nI do *not* guarantee that this works. It worked for me single test-case, \nbut I think somebody should double-check my logic. Does it seem sane?\n\nIt basically considers all the continuation lines to be lines of their \nown, and if the total line is bigger than what we can fit in it, we just \ntruncate the result rather than stop in the middle and then get confused \nwhen we try to parse the \"next\" line (which is just the remainder of the \nfirst line).\n\nIt also moves the \"From \" and \">From \" special cases into the rfc2822 \nheader line checker helper function, because the flow seemed much more \nlogical that way.\n\nIs this better than my stupid one-liner that just extended the line size? \nI dunno..\n\n\t\tLinus\n\n---\n builtin-mailinfo.c |   61 ++++++++++++++++++++++++++++++++++++---------------\n 1 files changed, 43 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 6ee6b0b..6ccbf75 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -406,6 +406,11 @@ static int is_rfc2822_header(char *line)\n \t */\n \tint ch;\n \tchar *cp = line;\n+\n+\t/* Count mbox From headers as headers */\n+\tif (!memcmp(line, \"From \", 5) || !memcmp(line, \">From \", 6))\n+\t\treturn 1;\n+\n \twhile ((ch = *cp++)) {\n \t\tif (ch == ':')\n \t\t\treturn cp != line;\n@@ -419,28 +424,48 @@ static int is_rfc2822_header(char *line)\n \n static int read_one_header_line(char *line, int sz, FILE *in)\n {\n-\tint ofs = 0;\n-\twhile (ofs < sz) {\n-\t\tint peek, len;\n-\t\tif (fgets(line + ofs, sz - ofs, in) == NULL)\n-\t\t\tbreak;\n-\t\tlen = eatspace(line + ofs);\n-\t\tif ((len == 0) || !is_rfc2822_header(line)) {\n-\t\t\t/* Re-add the newline */\n-\t\t\tline[ofs + len] = '\\n';\n-\t\t\tline[ofs + len + 1] = '\\0';\n-\t\t\tbreak;\n-\t\t}\n-\t\tofs += len;\n-\t\t/* Yuck, 2822 header \"folding\" */\n+\tint len;\n+\n+\t/* Get the first part of the line.. */\n+\tif (!fgets(line, sz, in))\n+\t\treturn 0;\n+\n+\t/*\n+\t * Is it an empty line or not a valid rfc2822 header?\n+\t * If so, stop here, and return false (\"not a header\")\n+\t */\n+\tlen = eatspace(line);\n+\tif (!len || !is_rfc2822_header(line)) {\n+\t\t/* Re-add the newline */\n+\t\tline[len] = '\\n';\n+\t\tline[len + 1] = '\\0';\n+\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Now we need to eat all the continuation lines..\n+\t * Yuck, 2822 header \"folding\"\n+\t */\n+\tfor (;;) {\n+\t\tint peek, addlen;\n+\t\tstatic char continuation[1000];\n+\n \t\tpeek = fgetc(in); ungetc(peek, in);\n \t\tif (peek != ' ' && peek != '\\t')\n \t\t\tbreak;\n+\t\tif (!fgets(continuation, sizeof(continuation), in))\n+\t\t\tbreak;\n+\t\taddlen = eatspace(continuation);\n+\t\tif (len < sz) {\n+\t\t\tif (addlen >= sz - len)\n+\t\t\t\taddlen = sz - len - 1;\n+\t\t\tmemcpy(line + len, continuation, addlen);\n+\t\t\tlen += addlen;\n+\t\t}\n \t}\n-\t/* Count mbox From headers as headers */\n-\tif (!ofs && (!memcmp(line, \"From \", 5) || !memcmp(line, \">From \", 6)))\n-\t\tofs = 1;\n-\treturn ofs;\n+\tline[len] = 0;\n+\n+\treturn 1;\n }\n \n static int decode_q_segment(char *in, char *ot, char *ep, int rfc2047)\n"},{"id":"35527","messageId":"Pine.LNX.4.63.0702262020590.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6975","inReplyTo":"Pine.LNX.4.64.0702261107060.12485@woody.linux-foundation.org","subject":"Re: Increase some limits in git-mailinfo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T19:34:22Z","receivedAt":"2007-02-26T19:34:22Z","isPatch":false,"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> +\n> +\t/* Count mbox From headers as headers */\n> +\tif (!memcmp(line, \"From \", 5) || !memcmp(line, \">From \", 6))\n\nI know that you copied that, but why not use the new prefixcmp() function?\n\n>  static int read_one_header_line(char *line, int sz, FILE *in)\n>  {\n> -\tint ofs = 0;\n> -\twhile (ofs < sz) {\n> -\t\tint peek, len;\n> -\t\tif (fgets(line + ofs, sz - ofs, in) == NULL)\n> -\t\t\tbreak;\n> -\t\tlen = eatspace(line + ofs);\n> -\t\tif ((len == 0) || !is_rfc2822_header(line)) {\n> -\t\t\t/* Re-add the newline */\n> -\t\t\tline[ofs + len] = '\\n';\n> -\t\t\tline[ofs + len + 1] = '\\0';\n> -\t\t\tbreak;\n> -\t\t}\n> -\t\tofs += len;\n> -\t\t/* Yuck, 2822 header \"folding\" */\n> +\tint len;\n> +\n> +\t/* Get the first part of the line.. */\n> +\tif (!fgets(line, sz, in))\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * Is it an empty line or not a valid rfc2822 header?\n> +\t * If so, stop here, and return false (\"not a header\")\n> +\t */\n> +\tlen = eatspace(line);\n> +\tif (!len || !is_rfc2822_header(line)) {\n> +\t\t/* Re-add the newline */\n> +\t\tline[len] = '\\n';\n> +\t\tline[len + 1] = '\\0';\n\nJust a micro-issue: if the input ends in the middle of a header line, \n_and_ this line is of exactly the right size (sz), then len + 1 is not \nnecessarily smaller than sz, right?\n\nIt's not like this can be used for a DOS or stuff, but I try to get more \naware of off-by-one bugs, which I introduced all too often...\n\n> +\t\taddlen = eatspace(continuation);\n> +\t\tif (len < sz) {\n\nShouldn't this be \"sz - 1\"? We are setting \"line[len] = 0;\" later... While \nat it, we can always check for \"addlen > 0\", just in case?\n\nBut then, should it not be rather \"sz - 2\", because we want to _add_ \nsomething to the line?\n\n> +\t\t\tif (addlen >= sz - len)\n> +\t\t\t\taddlen = sz - len - 1;\n> +\t\t\tmemcpy(line + len, continuation, addlen);\n> +\t\t\tlen += addlen;\n> +\t\t}\n>  \t}\n> -\t/* Count mbox From headers as headers */\n> -\tif (!ofs && (!memcmp(line, \"From \", 5) || !memcmp(line, \">From \", 6)))\n> -\t\tofs = 1;\n> -\treturn ofs;\n> +\tline[len] = 0;\n> +\n> +\treturn 1;\n\nCiao,\nDscho\n"},{"id":"35540","messageId":"Pine.LNX.4.64.0702261207520.12485@woody.linux-foundation.org","threadId":"6975","inReplyTo":"Pine.LNX.4.63.0702262020590.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Increase some limits in git-mailinfo","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T20:13:14Z","receivedAt":"2007-02-26T20:13:14Z","isPatch":false,"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> I know that you copied that, but why not use the new prefixcmp() function?\n\nHeh. Partly just because I copied it, partly because I'm not that used to \nthe interface (and partly becuase I hate how it uses the same complex \ninterface as 'strncmp()' does - I'd much rather see a _boolean_ return \nvalue, instead of the tristate one it uses. I doubt anybody would ever use \nit for sorting purposes anyway, and the tristate return value just results \nin code that is harder to read).\n\nSo I'd rather have it return true if it matches, rather than false if it \nmatches and -1/1 if it doesn't. But that's water under the bridge.\n\n\n> > +\t/* Get the first part of the line.. */\n> > +\tif (!fgets(line, sz, in))\n> > +\t\treturn 0;\n> > +\n> > +\t/*\n> > +\t * Is it an empty line or not a valid rfc2822 header?\n> > +\t * If so, stop here, and return false (\"not a header\")\n> > +\t */\n> > +\tlen = eatspace(line);\n> > +\tif (!len || !is_rfc2822_header(line)) {\n> > +\t\t/* Re-add the newline */\n> > +\t\tline[len] = '\\n';\n> > +\t\tline[len + 1] = '\\0';\n> \n> Just a micro-issue: if the input ends in the middle of a header line, \n> _and_ this line is of exactly the right size (sz), then len + 1 is not \n> necessarily smaller than sz, right?\n\nYeah. It's an old bug, though, the old code had the same behaviour.\n\nBut you're right. It might be better to have a\n\n\tassert(sz);\n\tsz--;\n\nat the top of the function, and just realize that sz is the size of the \nbuffer minus the size for the final '\\0', and that also indirectly would \nmean that even if the initial fgets() didn't have a '\\n' at the end, we \nalways have space to insert it.\n\n> > +\t\taddlen = eatspace(continuation);\n> > +\t\tif (len < sz) {\n> \n> Shouldn't this be \"sz - 1\"? We are setting \"line[len] = 0;\" later... While \n> at it, we can always check for \"addlen > 0\", just in case?\n\nYeah. Again, the \"sz--\" would solve that.\n\n\t\t\tLinus\n"},{"id":"35541","messageId":"Pine.LNX.4.63.0702262117160.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6975","inReplyTo":"Pine.LNX.4.64.0702261207520.12485@woody.linux-foundation.org","subject":"Re: Increase some limits in git-mailinfo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T20:18:54Z","receivedAt":"2007-02-26T20:18:54Z","isPatch":false,"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> So I'd rather have it return true if it matches, rather than false if it \n> matches and -1/1 if it doesn't. But that's water under the bridge.\n\nIn that case, you'd have to name it has_prefix() or hasprefix() rather \nthan prefixcmp(), which C programmers used to the semantics of strcmp() \nexpect to return a tristate result.\n\n> \tassert(sz);\n> \tsz--;\n\nMelikes.\n\nCiao,\nDscho\n"},{"id":"35543","messageId":"Pine.LNX.4.64.0702261221530.12485@woody.linux-foundation.org","threadId":"6975","inReplyTo":"Pine.LNX.4.63.0702262117160.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Increase some limits in git-mailinfo","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T20:22:26Z","receivedAt":"2007-02-26T20:22:26Z","isPatch":false,"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> > So I'd rather have it return true if it matches, rather than false if it \n> > matches and -1/1 if it doesn't. But that's water under the bridge.\n> \n> In that case, you'd have to name it has_prefix() or hasprefix() rather \n> than prefixcmp(), which C programmers used to the semantics of strcmp() \n> expect to return a tristate result.\n\nSure. Or \"match_prefix()\" or whatever. I agree.\n\n\t\tLinus\n"},{"id":"35545","messageId":"Pine.LNX.4.63.0702262135140.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6975","inReplyTo":"Pine.LNX.4.64.0702261221530.12485@woody.linux-foundation.org","subject":"Re: Increase some limits in git-mailinfo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-26T20:43:02Z","receivedAt":"2007-02-26T20:43:02Z","isPatch":false,"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> > > So I'd rather have it return true if it matches, rather than false \n> > > if it matches and -1/1 if it doesn't. But that's water under the \n> > > bridge.\n> > \n> > In that case, you'd have to name it has_prefix() or hasprefix() rather \n> > than prefixcmp(), which C programmers used to the semantics of \n> > strcmp() expect to return a tristate result.\n> \n> Sure. Or \"match_prefix()\" or whatever. I agree.\n\nWell, if you look at fnmatch(), or our own match_pathspec(), rather not...\n\nJust my 2 cents: I can live with prefixcmp(). And since I use strcmp() to \ncompare strings, not strings_eq() or similar, I am quite happy with \nprefixcmp(), too.\n\nCiao,\nDscho\n"},{"id":"35546","messageId":"Pine.LNX.4.64.0702261320350.12485@woody.linux-foundation.org","threadId":"6975","inReplyTo":"Pine.LNX.4.63.0702262135140.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: Increase some limits in git-mailinfo","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-02-26T21:21:19Z","receivedAt":"2007-02-26T21:21:19Z","isPatch":false,"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> Just my 2 cents: I can live with prefixcmp(). And since I use strcmp() to \n> compare strings, not strings_eq() or similar, I am quite happy with \n> prefixcmp(), too.\n\nOh, I can live with it too. This is not a big issue for me. I just \nexplained why I didn't use it, and why I would probably have done it \ndifferently had I done the prefixcmp() thing.\n\n\t\tLinus\n"}]}