# Increase some limits in git-mailinfo 8 messages from 2007-02-26 to 2007-02-26. Participants: Linus Torvalds, Johannes Schindelin. Thread: https://gitlist.dev/t/6975 ## Linus Torvalds, 2007-02-26 18:43 Subject: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702261036550.12485%40woody.linux-foundation.org ``` I'm not proud of this patch, and I think we should do this better, but I just had trouble with an email that Eric Biederman sent me for the kernel, and this is the simplest and most obvious fix.. What happened is that Eric's email had a long and complex "References:" line in his email headers, and the way git-mailinfo parses email headers, it will combine these multi-line headers into one single long line (which is appropriate for things like Subject lines etc, where everybody really wants to just have one long subject rather than know about rfc822 email message rules). However, with the extensive references, the end result was longer than the one thousand character line limit we had, and what git-mailinfo did was to just stop parsing header lines. The end result: it never parsed the "Date:" line at all, and it left all subsequent email header lines as "commit info" contents instead. Very ugly. Arguably that is the real bug (we should continue with header parsing), but the trivial work-around is to just make the line buffer bigger. Signed-off-by: Linus Torvalds --- I'll look at making the parsing more robust too, but thought I'd send this out asap. Linus --- builtin-mailinfo.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c index 6ee6b0b..3f0e08a 100644 --- a/builtin-mailinfo.c +++ b/builtin-mailinfo.c @@ -10,7 +10,7 @@ static FILE *cmitmsg, *patchfile, *fin, *fout; static int keep_subject; static const char *metainfo_charset; -static char line[1000]; +static char line[10000]; static char date[1000]; static char name[1000]; static char email[1000]; ``` ## Linus Torvalds, 2007-02-26 19:10 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702261107060.12485%40woody.linux-foundation.org In-Reply-To: ``` On Mon, 26 Feb 2007, Linus Torvalds wrote: > > I'm not proud of this patch, and I think we should do this better, but I > just had trouble with an email that Eric Biederman sent me for the kernel, > and this is the simplest and most obvious fix.. Ok, this is the more complicated fix for the problem. I do *not* guarantee that this works. It worked for me single test-case, but I think somebody should double-check my logic. Does it seem sane? It basically considers all the continuation lines to be lines of their own, and if the total line is bigger than what we can fit in it, we just truncate the result rather than stop in the middle and then get confused when we try to parse the "next" line (which is just the remainder of the first line). It also moves the "From " and ">From " special cases into the rfc2822 header line checker helper function, because the flow seemed much more logical that way. Is this better than my stupid one-liner that just extended the line size? I dunno.. Linus --- builtin-mailinfo.c | 61 ++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 43 insertions(+), 18 deletions(-) diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c index 6ee6b0b..6ccbf75 100644 --- a/builtin-mailinfo.c +++ b/builtin-mailinfo.c @@ -406,6 +406,11 @@ static int is_rfc2822_header(char *line) */ int ch; char *cp = line; + + /* Count mbox From headers as headers */ + if (!memcmp(line, "From ", 5) || !memcmp(line, ">From ", 6)) + return 1; + while ((ch = *cp++)) { if (ch == ':') return cp != line; @@ -419,28 +424,48 @@ static int is_rfc2822_header(char *line) static int read_one_header_line(char *line, int sz, FILE *in) { - int ofs = 0; - while (ofs < sz) { - int peek, len; - if (fgets(line + ofs, sz - ofs, in) == NULL) - break; - len = eatspace(line + ofs); - if ((len == 0) || !is_rfc2822_header(line)) { - /* Re-add the newline */ - line[ofs + len] = '\n'; - line[ofs + len + 1] = '\0'; - break; - } - ofs += len; - /* Yuck, 2822 header "folding" */ + int len; + + /* Get the first part of the line.. */ + if (!fgets(line, sz, in)) + return 0; + + /* + * Is it an empty line or not a valid rfc2822 header? + * If so, stop here, and return false ("not a header") + */ + len = eatspace(line); + if (!len || !is_rfc2822_header(line)) { + /* Re-add the newline */ + line[len] = '\n'; + line[len + 1] = '\0'; + return 0; + } + + /* + * Now we need to eat all the continuation lines.. + * Yuck, 2822 header "folding" + */ + for (;;) { + int peek, addlen; + static char continuation[1000]; + peek = fgetc(in); ungetc(peek, in); if (peek != ' ' && peek != '\t') break; + if (!fgets(continuation, sizeof(continuation), in)) + break; + addlen = eatspace(continuation); + if (len < sz) { + if (addlen >= sz - len) + addlen = sz - len - 1; + memcpy(line + len, continuation, addlen); + len += addlen; + } } - /* Count mbox From headers as headers */ - if (!ofs && (!memcmp(line, "From ", 5) || !memcmp(line, ">From ", 6))) - ofs = 1; - return ofs; + line[len] = 0; + + return 1; } static int decode_q_segment(char *in, char *ot, char *ep, int rfc2047) ``` ## Johannes Schindelin, 2007-02-26 19:34 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702262020590.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Mon, 26 Feb 2007, Linus Torvalds wrote: > + > + /* Count mbox From headers as headers */ > + if (!memcmp(line, "From ", 5) || !memcmp(line, ">From ", 6)) I know that you copied that, but why not use the new prefixcmp() function? > static int read_one_header_line(char *line, int sz, FILE *in) > { > - int ofs = 0; > - while (ofs < sz) { > - int peek, len; > - if (fgets(line + ofs, sz - ofs, in) == NULL) > - break; > - len = eatspace(line + ofs); > - if ((len == 0) || !is_rfc2822_header(line)) { > - /* Re-add the newline */ > - line[ofs + len] = '\n'; > - line[ofs + len + 1] = '\0'; > - break; > - } > - ofs += len; > - /* Yuck, 2822 header "folding" */ > + int len; > + > + /* Get the first part of the line.. */ > + if (!fgets(line, sz, in)) > + return 0; > + > + /* > + * Is it an empty line or not a valid rfc2822 header? > + * If so, stop here, and return false ("not a header") > + */ > + len = eatspace(line); > + if (!len || !is_rfc2822_header(line)) { > + /* Re-add the newline */ > + line[len] = '\n'; > + line[len + 1] = '\0'; Just a micro-issue: if the input ends in the middle of a header line, _and_ this line is of exactly the right size (sz), then len + 1 is not necessarily smaller than sz, right? It's not like this can be used for a DOS or stuff, but I try to get more aware of off-by-one bugs, which I introduced all too often... > + addlen = eatspace(continuation); > + if (len < sz) { Shouldn't this be "sz - 1"? We are setting "line[len] = 0;" later... While at it, we can always check for "addlen > 0", just in case? But then, should it not be rather "sz - 2", because we want to _add_ something to the line? > + if (addlen >= sz - len) > + addlen = sz - len - 1; > + memcpy(line + len, continuation, addlen); > + len += addlen; > + } > } > - /* Count mbox From headers as headers */ > - if (!ofs && (!memcmp(line, "From ", 5) || !memcmp(line, ">From ", 6))) > - ofs = 1; > - return ofs; > + line[len] = 0; > + > + return 1; Ciao, Dscho ``` ## Linus Torvalds, 2007-02-26 20:13 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702261207520.12485%40woody.linux-foundation.org In-Reply-To: ``` On Mon, 26 Feb 2007, Johannes Schindelin wrote: > > I know that you copied that, but why not use the new prefixcmp() function? Heh. Partly just because I copied it, partly because I'm not that used to the interface (and partly becuase I hate how it uses the same complex interface as 'strncmp()' does - I'd much rather see a _boolean_ return value, instead of the tristate one it uses. I doubt anybody would ever use it for sorting purposes anyway, and the tristate return value just results in code that is harder to read). So I'd rather have it return true if it matches, rather than false if it matches and -1/1 if it doesn't. But that's water under the bridge. > > + /* Get the first part of the line.. */ > > + if (!fgets(line, sz, in)) > > + return 0; > > + > > + /* > > + * Is it an empty line or not a valid rfc2822 header? > > + * If so, stop here, and return false ("not a header") > > + */ > > + len = eatspace(line); > > + if (!len || !is_rfc2822_header(line)) { > > + /* Re-add the newline */ > > + line[len] = '\n'; > > + line[len + 1] = '\0'; > > Just a micro-issue: if the input ends in the middle of a header line, > _and_ this line is of exactly the right size (sz), then len + 1 is not > necessarily smaller than sz, right? Yeah. It's an old bug, though, the old code had the same behaviour. But you're right. It might be better to have a assert(sz); sz--; at the top of the function, and just realize that sz is the size of the buffer minus the size for the final '\0', and that also indirectly would mean that even if the initial fgets() didn't have a '\n' at the end, we always have space to insert it. > > + addlen = eatspace(continuation); > > + if (len < sz) { > > Shouldn't this be "sz - 1"? We are setting "line[len] = 0;" later... While > at it, we can always check for "addlen > 0", just in case? Yeah. Again, the "sz--" would solve that. Linus ``` ## Johannes Schindelin, 2007-02-26 20:18 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702262117160.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Mon, 26 Feb 2007, Linus Torvalds wrote: > So I'd rather have it return true if it matches, rather than false if it > matches and -1/1 if it doesn't. But that's water under the bridge. In that case, you'd have to name it has_prefix() or hasprefix() rather than prefixcmp(), which C programmers used to the semantics of strcmp() expect to return a tristate result. > assert(sz); > sz--; Melikes. Ciao, Dscho ``` ## Linus Torvalds, 2007-02-26 20:22 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702261221530.12485%40woody.linux-foundation.org In-Reply-To: ``` On Mon, 26 Feb 2007, Johannes Schindelin wrote: > > > So I'd rather have it return true if it matches, rather than false if it > > matches and -1/1 if it doesn't. But that's water under the bridge. > > In that case, you'd have to name it has_prefix() or hasprefix() rather > than prefixcmp(), which C programmers used to the semantics of strcmp() > expect to return a tristate result. Sure. Or "match_prefix()" or whatever. I agree. Linus ``` ## Johannes Schindelin, 2007-02-26 20:43 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702262135140.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: ``` Hi, On Mon, 26 Feb 2007, Linus Torvalds wrote: > On Mon, 26 Feb 2007, Johannes Schindelin wrote: > > > > > So I'd rather have it return true if it matches, rather than false > > > if it matches and -1/1 if it doesn't. But that's water under the > > > bridge. > > > > In that case, you'd have to name it has_prefix() or hasprefix() rather > > than prefixcmp(), which C programmers used to the semantics of > > strcmp() expect to return a tristate result. > > Sure. Or "match_prefix()" or whatever. I agree. Well, if you look at fnmatch(), or our own match_pathspec(), rather not... Just my 2 cents: I can live with prefixcmp(). And since I use strcmp() to compare strings, not strings_eq() or similar, I am quite happy with prefixcmp(), too. Ciao, Dscho ``` ## Linus Torvalds, 2007-02-26 21:21 Subject: Re: Increase some limits in git-mailinfo Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702261320350.12485%40woody.linux-foundation.org In-Reply-To: ``` On Mon, 26 Feb 2007, Johannes Schindelin wrote: > > Just my 2 cents: I can live with prefixcmp(). And since I use strcmp() to > compare strings, not strings_eq() or similar, I am quite happy with > prefixcmp(), too. Oh, I can live with it too. This is not a big issue for me. I just explained why I didn't use it, and why I would probably have done it differently had I done the prefixcmp() thing. Linus ```