{"thread":{"id":"9711","subject":"Buffer overflows","startedAt":"2007-08-30T19:26:49Z","lastAt":"2007-09-03T00:31:14Z","messageCount":26,"participants":["Timo Sirainen","Lukas Sandström","Linus Torvalds","Reece Dunn","Alex Riesen","Junio C Hamano","Pierre Habouzit","Andreas Ericsson","Johannes Schindelin","Wincent Colaiuta","Simon 'corecode' Schubert","Johan Herland","David Kastrup","René Scharfe","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"51934","messageId":"1188502009.29782.874.camel@hurina","threadId":"9711","inReplyTo":null,"subject":"Buffer overflows","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-08-30T19:26:49Z","receivedAt":"2007-08-30T19:26:49Z","isPatch":false,"sender":{"key":"tss@iki.fi","avatar":null},"body":"Looks like nothing has happened since my last mail about this\n(http://marc.info/?l=git&m=117962988804430&w=2).\n\nI sure hope no-one's using git-mailinfo to do any kind of automated mail\nprocessing from untrusted users. Here's one way to cause it to overflow\na buffer in stack:\n\nSubject: =?iso-8859-15?b?pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSk\n pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSk\n pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSk\n pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKQ=?=\n\nIt's not the only way. I just accidentally hit that when trying to\nverify another buffer overflow. Just look for strcpy()s in the file.\n\nLibc string handling functions are broken and should not be used for\nanything. It's annoying when such a large project as Git is still\nrepeating the same old mistakes. I guess security doesn't matter to\nanyone.\n\nAttached once again beginnings of safer string handling functions, which\nshould be easy to use to replace the existing string handling code. I\neven thought about creating some kind of an automated tool to do this,\nbut that's a bit too much trouble with no gain for myself.\n\nUsage goes like:\n\nSTATIC_STRING(str, 1024);\nsstr_append(str, \"hello \");\nsstr_printfa(str, \"%d\", 5);\n\nstruct string *str;\nstr = str_alloc(1024); // initial malloc size, grows when needed\nstr_append(str, \"hello \");\nstr_printfa(str, \"%d\", 5);\nstr_free(&str);\n\n\n\ndiff --git a/Makefile b/Makefile\nindex 4eb4637..c79eced 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -135,7 +135,7 @@ uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')\n \n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -Wall\n LDFLAGS =\n ALL_CFLAGS = $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -283,7 +283,7 @@ XDIFF_LIB=xdiff/lib.a\n LIB_H = \\\n \tarchive.h blob.h cache.h commit.h csum-file.h delta.h grep.h \\\n \tdiff.h object.h pack.h pkt-line.h quote.h refs.h list-objects.h sideband.h \\\n-\trun-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \\\n+\trun-command.h strbuf.h str.h str-static.h tag.h tree.h git-compat-util.h revision.h \\\n \ttree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \\\n \tutf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \\\n \tmailmap.h remote.h\n@@ -302,7 +302,7 @@ LIB_OBJS = \\\n \tobject.o pack-check.o pack-write.o patch-delta.o path.o pkt-line.o \\\n \tsideband.o reachable.o reflog-walk.o \\\n \tquote.o read-cache.o refs.o run-command.o dir.o object-refs.o \\\n-\tserver-info.o setup.o sha1_file.o sha1_name.o strbuf.o \\\n+\tserver-info.o setup.o sha1_file.o sha1_name.o strbuf.o str.o str-static.o \\\n \ttag.o tree.o usage.o config.o environment.o ctype.o copy.o \\\n \trevision.o pager.o tree-walk.o xdiff-interface.o \\\n \twrite_or_die.o trace.o list-objects.o grep.o match-trees.o \\\ndiff --git a/str-static.c b/str-static.c\nnew file mode 100644\nindex 0000000..a3f48f8\n--- /dev/null\n+++ b/str-static.c\n@@ -0,0 +1,40 @@\n+#include \"str-static.h\"\n+\n+void str_static_append(struct string_static *str, const char *cstr)\n+{\n+\tunsigned int avail = str->size - str->len;\n+\tunsigned int len = strlen(cstr);\n+\n+\tif (len >= avail) {\n+\t\tlen = avail - 1;\n+\t\tstr->overflowed = 1;\n+\t}\n+\tmemcpy(str->buf + str->len, cstr, len);\n+\tstr->len += len;\n+\tstr->buf[str->len] = '\\0';\n+}\n+\n+void str_static_printfa(struct string_static *str, const char *fmt, ...)\n+{\n+\tunsigned int avail = str->size - str->len;\n+\tva_list va;\n+\tint ret;\n+\n+\tva_start(va, fmt);\n+\tret = vsnprintf(str->buf + str->len, avail, fmt, va);\n+\tif (ret < (int)avail)\n+\t\tstr->len += ret;\n+\telse {\n+\t\tstr->len += avail - 1;\n+\t\tstr->overflowed = 1;\n+\t}\n+\tva_end(va);\n+}\n+\n+void str_static_truncate(struct string_static *str, unsigned int len)\n+{\n+\tif (len >= str->size)\n+\t\tlen = str->size - 1;\n+\tstr->len = len;\n+\tstr->buf[len] = '\\0';\n+}\ndiff --git a/str-static.h b/str-static.h\nnew file mode 100644\nindex 0000000..19cb28f\n--- /dev/null\n+++ b/str-static.h\n@@ -0,0 +1,33 @@\n+#ifndef STR_STATIC_H\n+#define STR_STATIC_H\n+\n+#include \"git-compat-util.h\"\n+\n+struct string_static {\n+\tunsigned int size;\n+\tunsigned int len:31;\n+\tunsigned int overflowed:1;\n+\tchar buf[];\n+};\n+\n+#define STR_STATIC(name, size) \\\n+\tunion { \\\n+\t  struct string_static string; \\\n+\t  char string_buf[sizeof(struct string_static) + (size) + 1]; \\\n+\t} name = { { (size)+1, 0, 0 } }\n+\n+extern void str_static_append(struct string_static *str, const char *cstr);\n+extern void str_static_printfa(struct string_static *str, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+extern void str_static_truncate(struct string_static *str, unsigned int len);\n+\n+#define sstr_append(str, cstr) str_static_append(&(str).string, cstr)\n+#define sstr_printfa(str, fmt, ...) \\\n+\tstr_static_printfa(&(str).string, fmt, __VA_ARGS__)\n+#define sstr_truncate(str, len) str_static_truncate(&(str).string, len)\n+\n+#define sstr_c(str) ((str).string.buf)\n+#define sstr_len(str) ((str).string.len)\n+#define sstr_overflowed(str) ((str).string.overflowed)\n+\n+#endif\ndiff --git a/str.c b/str.c\nnew file mode 100644\nindex 0000000..7530073\n--- /dev/null\n+++ b/str.c\n@@ -0,0 +1,75 @@\n+#include \"str.h\"\n+#include \"git-compat-util.h\"\n+\n+extern struct string *str_alloc(unsigned int initial_size)\n+{\n+\tstruct string *str;\n+\n+\tstr = xmalloc(sizeof(*str));\n+\tstr->len = 0;\n+\tstr->size = initial_size + 1;\n+\tstr->buf = xmalloc(str->size);\n+\tstr->buf[0] = '\\0';\n+\treturn str;\n+}\n+\n+void str_free(struct string **_str)\n+{\n+\tstruct string *str = *_str;\n+\n+\tfree(str->buf);\n+\tstr->buf = NULL;\n+\tfree(str);\n+\n+\t*_str = NULL;\n+}\n+\n+static void str_grow_if_needed(struct string *str, unsigned int len)\n+{\n+\tunsigned int avail = str->size - str->len;\n+\n+\tif (len >= avail) {\n+\t\tstr->size = (str->len + len) * 2;\n+\t\tstr->buf = xrealloc(str->buf, str->size);\n+\t}\n+}\n+\n+void str_append(struct string *str, const char *cstr)\n+{\n+\tunsigned int len = strlen(cstr);\n+\n+\tstr_grow_if_needed(str, len);\n+\tmemcpy(str->buf + str->len, cstr, len);\n+\tstr->len += len;\n+\tstr->buf[str->len] = '\\0';\n+}\n+\n+void str_printfa(struct string *str, const char *fmt, ...)\n+{\n+\tunsigned int avail = str->size - str->len;\n+\tva_list va, va2;\n+\tint ret;\n+\n+\tva_start(va, fmt);\n+\tva_copy(va2, va);\n+\tret = vsnprintf(str->buf + str->len, avail, fmt, va);\n+\tassert(ret >= 0);\n+\n+\tif ((unsigned int)ret >= avail) {\n+\t\tstr_grow_if_needed(str, ret);\n+\t\tavail = str->size - str->len;\n+\n+\t\tret = vsnprintf(str->buf + str->len, avail, fmt, va2);\n+\t\tassert(ret >= 0 && (unsigned int)ret < avail);\n+\t}\n+\tstr->len += ret;\n+\tva_end(va);\n+}\n+\n+void str_truncate(struct string *str, unsigned int len)\n+{\n+\tif (len >= str->size)\n+\t\tlen = str->size - 1;\n+\tstr->len = len;\n+\tstr->buf[len] = '\\0';\n+}\ndiff --git a/str.h b/str.h\nnew file mode 100644\nindex 0000000..329aad4\n--- /dev/null\n+++ b/str.h\n@@ -0,0 +1,20 @@\n+#ifndef STR_H\n+#define STR_H\n+\n+struct string {\n+\tunsigned int len, size;\n+\tchar *buf;\n+};\n+\n+extern struct string *str_alloc(unsigned int initial_size);\n+extern void str_free(struct string **str);\n+\n+extern void str_append(struct string *str, const char *cstr);\n+extern void str_printfa(struct string *str, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+extern void str_truncate(struct string *str, unsigned int len);\n+\n+#define str_c(str) ((str)->buf)\n+#define str_len(str) ((str)->len)\n+\n+#endif\n"},{"id":"51936","messageId":"46D727E6.1060006@etek.chalmers.se","threadId":"9711","inReplyTo":"1188502009.29782.874.camel@hurina","subject":"Re: Buffer overflows","fromName":"Lukas Sandström","fromEmail":"lukass@etek.chalmers.se","sentAt":"2007-08-30T20:26:14Z","receivedAt":"2007-08-30T20:26:14Z","isPatch":false,"sender":{"key":"luksan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152281?v=4"},"body":"Timo Sirainen wrote:\n> Attached once again beginnings of safer string handling functions, which\n> should be easy to use to replace the existing string handling code. I\n> even thought about creating some kind of an automated tool to do this,\n> but that's a bit too much trouble with no gain for myself.\n\nHow about using an existing string handling library instead of\ncreating another one from scratch?\n\nOne library worth looking at might be \"The Better String Library\"[1].\n\nIt claims to be both portable, stable, secure and have high performance.\n\n/Lukas\n\n[1] http://bstring.sourceforge.net/\n"},{"id":"51941","messageId":"alpine.LFD.0.999.0708301340470.25853@woody.linux-foundation.org","threadId":"9711","inReplyTo":"1188502009.29782.874.camel@hurina","subject":"Re: Buffer overflows","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-30T20:46:32Z","receivedAt":"2007-08-30T20:46:32Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 30 Aug 2007, Timo Sirainen wrote:\n>\n> Looks like nothing has happened since my last mail about this\n> (http://marc.info/?l=git&m=117962988804430&w=2).\n\nPerhaps because your patch was using a totally nonstandard and slow \ninterface, and had nasty string declaration issues, as people even pointed \nout to you.\n\nIf you were to send in a patch that simply just fixed some random case \nwithout introducing the other stuff in forms that nobody is used to, \npeople would probably react more.\n\nEspecially since:\n\n> I sure hope no-one's using git-mailinfo to do any kind of automated mail \n> processing from untrusted users.\n\nObviously nobody would do that. Not because of any email buffer overflows, \nbut because people wouldn't want to apply untrusted patches in the first \nplace!\n\nIOW, if I don't trust the mail, I'd sure as hell not apply it - not \nbecause I'm afraid the email is misformed, but because I'd not trust the \n*patch*.\n\n\t\t\tLinus\n"},{"id":"51943","messageId":"7D84F3C7-129D-4197-AAF1-46298E5D0136@iki.fi","threadId":"9711","inReplyTo":"alpine.LFD.0.999.0708301340470.25853@woody.linux-foundation.org","subject":"Re: Buffer overflows","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-08-30T21:08:22Z","receivedAt":"2007-08-30T21:08:22Z","isPatch":false,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On 30.8.2007, at 23.46, Linus Torvalds wrote:\n\n> On Thu, 30 Aug 2007, Timo Sirainen wrote:\n>>\n>> Looks like nothing has happened since my last mail about this\n>> (http://marc.info/?l=git&m=117962988804430&w=2).\n>\n> Perhaps because your patch was using a totally nonstandard and slow\n> interface, and had nasty string declaration issues, as people even  \n> pointed\n> out to you.\n\nSlow?\n\n> If you were to send in a patch that simply just fixed some random case\n> without introducing the other stuff in forms that nobody is used to,\n> people would probably react more.\n\nThe problem is that the git code is full of these random cases. It's  \nsimply a huge job to even try to verify the correctness of it. Even  \nif someone did that and fixed all the problems, tomorrow there would  \nbe new ones because noone bothers to even try to avoid them. So there  \nreally isn't any point in trying to make git secure until the coding  \nstyle changes.\n\nThe code should be easy to verify to be secure, and with some kind of  \na safe string API it's a lot easier than trying to figure out corner  \ncases where strcpy() calls break.\n\n> Especially since:\n>\n>> I sure hope no-one's using git-mailinfo to do any kind of  \n>> automated mail\n>> processing from untrusted users.\n>\n> Obviously nobody would do that. Not because of any email buffer  \n> overflows,\n> but because people wouldn't want to apply untrusted patches in the  \n> first\n> place!\n\nAnd anyone who uses git-mailinfo for anything else than manually  \napplying trusted patches to their own tree deserve what they get, I  \nsuppose? For example if I decided to use it to automatically extract  \npatches and their descriptions out of received emails and put them in  \na queue somewhere where I could look at them more easily.\n"},{"id":"51945","messageId":"3f4fd2640708301435s7067137cp5db6334af844158a@mail.gmail.com","threadId":"9711","inReplyTo":"7D84F3C7-129D-4197-AAF1-46298E5D0136@iki.fi","subject":"Re: Buffer overflows","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2007-08-30T21:35:19Z","receivedAt":"2007-08-30T21:35:19Z","isPatch":false,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"On 30/08/2007, Timo Sirainen <tss@iki.fi> wrote:\n> On 30.8.2007, at 23.46, Linus Torvalds wrote:\n>\n> > On Thu, 30 Aug 2007, Timo Sirainen wrote:\n> >>\n> >> Looks like nothing has happened since my last mail about this\n> >> (http://marc.info/?l=git&m=117962988804430&w=2).\n>\n> > If you were to send in a patch that simply just fixed some random case\n> > without introducing the other stuff in forms that nobody is used to,\n> > people would probably react more.\n>\n> The problem is that the git code is full of these random cases. It's\n> simply a huge job to even try to verify the correctness of it. Even\n> if someone did that and fixed all the problems, tomorrow there would\n> be new ones because noone bothers to even try to avoid them. So there\n> really isn't any point in trying to make git secure until the coding\n> style changes.\n\nYou don't want a manual check to do these kinds of checks. Not only is\nit a huge job, you have the human factor: people make mistakes. This\nis (in part) what the review process is for, but understanding how to\nidentify code that is safe from buffer overruns, integer overflows and\nthe like is a complex task. Also, it may work on 32-bit which has been\nverified, but not on 64-bit.\n\nIt would be far better to specify the rules on how to detect these\nissues into a static analysis tool and have that do the checking for\nyou. Therefore, it is possible to detect when new problems have been\nadded into the codebase. Does sparse support identifying these issues?\n\n> The code should be easy to verify to be secure, and with some kind of\n> a safe string API it's a lot easier than trying to figure out corner\n> cases where strcpy() calls break.\n\nWhy is it easier? If you have a fixed-size buffer, why not use\nstrncpy, which is what a safe string API is essentially doing anyway?\n\nIn this case, detecting strcpy usage can be done via grep. This is\nquick, simple and easy to repeat. Other things are more complicated,\nwhich is where automated verification tools help.\n\n- Reece\n"},{"id":"51947","messageId":"20070830214824.GC15405@steel.home","threadId":"9711","inReplyTo":"1188502009.29782.874.camel@hurina","subject":"[PATCH] Temporary fix for stack smashing in mailinfo","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-30T21:48:24Z","receivedAt":"2007-08-30T21:48:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\nTimo Sirainen, Thu, Aug 30, 2007 21:26:49 +0200:\n> Attached once again beginnings of safer string handling functions, which\n> should be easy to use to replace the existing string handling code. ...\n\nLots of code. And the bug you mentioned still not fixed.\nNo gain for anyone. You are right, but you are also useless.\n\nJunio, I cannot have time to fix the code nice and proper, but as\nheavy user of git-am just have to have it fixed at least a like this.\nAnd this is ugly (and definitely incomplete), everyone be warned.\n\nChecked with valgrind, looks good (except for iconv_open reading past\none of its arguments):\n\n==22856== Invalid read of size 4\n...\n==22856==  Address 0x42D3D9C is 52 bytes inside a block of size 53 alloc'd\n==22856==    at 0x4021620: malloc (vg_replace_malloc.c:149)\n==22856==    by 0x41AD12F: (within /lib/tls/i686/cmov/libc-2.5.so)\n==22856==    by 0x41AC56A: (within /lib/tls/i686/cmov/libc-2.5.so)\n==22856==    by 0x41ACC63: (within /lib/tls/i686/cmov/libc-2.5.so)\n==22856==    by 0x41A552B: (within /lib/tls/i686/cmov/libc-2.5.so)\n==22856==    by 0x41A4093: (within /lib/tls/i686/cmov/libc-2.5.so)\n==22856==    by 0x41A3CF9: iconv_open (in /lib/tls/i686/cmov/libc-2.5.so)\n==22856==    by 0x80BEFFE: reencode_string (utf8.c:317)\n==22856==    by 0x806A9D4: convert_to_utf8 (builtin-mailinfo.c:543)\n==22856==    by 0x806AB8F: decode_header (builtin-mailinfo.c:600)\n==22856==    by 0x806AF57: check_header (builtin-mailinfo.c:308)\n==22856==    by 0x806B70B: cmd_mailinfo (builtin-mailinfo.c:936)\n\n builtin-mailinfo.c |   79 +++++++++++++++++++++++++++++----------------------\n 1 files changed, 45 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex b558754..57699eb 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -287,12 +287,12 @@ static void cleanup_space(char *buf)\n \t}\n }\n \n-static void decode_header(char *it);\n+static void decode_header(char *it, unsigned itsize);\n static char *header[MAX_HDR_PARSED] = {\n \t\"From\",\"Subject\",\"Date\",\n };\n \n-static int check_header(char *line, char **hdr_data, int overwrite)\n+static int check_header(char *line, unsigned linesize, char **hdr_data, int overwrite)\n {\n \tint i;\n \n@@ -305,7 +305,7 @@ static int check_header(char *line, char **hdr_data, int overwrite)\n \t\t\t/* Unwrap inline B and Q encoding, and optionally\n \t\t\t * normalize the meta information to utf8.\n \t\t\t */\n-\t\t\tdecode_header(line + len + 2);\n+\t\t\tdecode_header(line + len + 2, linesize - len - 2);\n \t\t\thdr_data[i] = xmalloc(1000 * sizeof(char));\n \t\t\tif (! handle_header(line, hdr_data[i], len + 2)) {\n \t\t\t\treturn 1;\n@@ -316,14 +316,14 @@ static int check_header(char *line, char **hdr_data, int overwrite)\n \t/* Content stuff */\n \tif (!strncasecmp(line, \"Content-Type\", 12) &&\n \t\tline[12] == ':' && isspace(line[12 + 1])) {\n-\t\tdecode_header(line + 12 + 2);\n+\t\tdecode_header(line + 12 + 2, linesize - 12 - 2);\n \t\tif (! handle_content_type(line)) {\n \t\t\treturn 1;\n \t\t}\n \t}\n \tif (!strncasecmp(line, \"Content-Transfer-Encoding\", 25) &&\n \t\tline[25] == ':' && isspace(line[25 + 1])) {\n-\t\tdecode_header(line + 25 + 2);\n+\t\tdecode_header(line + 25 + 2, linesize - 25 - 2);\n \t\tif (! handle_content_transfer_encoding(line)) {\n \t\t\treturn 1;\n \t\t}\n@@ -432,10 +432,15 @@ static int read_one_header_line(char *line, int sz, FILE *in)\n \treturn 1;\n }\n \n-static int decode_q_segment(char *in, char *ot, char *ep, int rfc2047)\n+static int decode_q_segment(char *in, char *ot, unsigned otsize, char *ep, int rfc2047)\n {\n+\tchar *otend = ot + otsize;\n \tint c;\n \twhile ((c = *in++) != 0 && (in <= ep)) {\n+\t\tif (ot == otend) {\n+\t\t\t*--ot = '\\0';\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (c == '=') {\n \t\t\tint d = *in++;\n \t\t\tif (d == '\\n' || !d)\n@@ -451,12 +456,17 @@ static int decode_q_segment(char *in, char *ot, char *ep, int rfc2047)\n \treturn 0;\n }\n \n-static int decode_b_segment(char *in, char *ot, char *ep)\n+static int decode_b_segment(char *in, char *ot, unsigned otsize, char *ep)\n {\n \t/* Decode in..ep, possibly in-place to ot */\n \tint c, pos = 0, acc = 0;\n+\tchar *otend = ot + otsize;\n \n \twhile ((c = *in++) != 0 && (in <= ep)) {\n+\t\tif (ot == otend) {\n+\t\t\t*--ot = '\\0';\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (c == '+')\n \t\t\tc = 62;\n \t\telse if (c == '/')\n@@ -518,7 +528,7 @@ static const char *guess_charset(const char *line, const char *target_charset)\n \treturn \"latin1\";\n }\n \n-static void convert_to_utf8(char *line, const char *charset)\n+static void convert_to_utf8(char *line, unsigned linesize, const char *charset)\n {\n \tchar *out;\n \n@@ -534,11 +544,11 @@ static void convert_to_utf8(char *line, const char *charset)\n \tif (!out)\n \t\tdie(\"cannot convert from %s to %s\\n\",\n \t\t    charset, metainfo_charset);\n-\tstrcpy(line, out);\n+\tstrlcpy(line, out, linesize);\n \tfree(out);\n }\n \n-static int decode_header_bq(char *it)\n+static int decode_header_bq(char *it, unsigned itsize)\n {\n \tchar *in, *out, *ep, *cp, *sp;\n \tchar outbuf[1000];\n@@ -578,56 +588,56 @@ static int decode_header_bq(char *it)\n \t\tdefault:\n \t\t\treturn rfc2047; /* no munging */\n \t\tcase 'b':\n-\t\t\tsz = decode_b_segment(cp + 3, piecebuf, ep);\n+\t\t\tsz = decode_b_segment(cp + 3, piecebuf, sizeof(piecebuf), ep);\n \t\t\tbreak;\n \t\tcase 'q':\n-\t\t\tsz = decode_q_segment(cp + 3, piecebuf, ep, 1);\n+\t\t\tsz = decode_q_segment(cp + 3, piecebuf, sizeof(piecebuf), ep, 1);\n \t\t\tbreak;\n \t\t}\n \t\tif (sz < 0)\n \t\t\treturn rfc2047;\n \t\tif (metainfo_charset)\n-\t\t\tconvert_to_utf8(piecebuf, charset_q);\n+\t\t\tconvert_to_utf8(piecebuf, sizeof(piecebuf), charset_q);\n \t\tstrcpy(out, piecebuf);\n \t\tout += strlen(out);\n \t\tin = ep + 2;\n \t}\n \tstrcpy(out, in);\n-\tstrcpy(it, outbuf);\n+\tstrlcpy(it, outbuf, itsize);\n \treturn rfc2047;\n }\n \n-static void decode_header(char *it)\n+static void decode_header(char *it, unsigned itsize)\n {\n \n-\tif (decode_header_bq(it))\n+\tif (decode_header_bq(it, itsize))\n \t\treturn;\n \t/* otherwise \"it\" is a straight copy of the input.\n \t * This can be binary guck but there is no charset specified.\n \t */\n \tif (metainfo_charset)\n-\t\tconvert_to_utf8(it, \"\");\n+\t\tconvert_to_utf8(it, itsize, \"\");\n }\n \n-static void decode_transfer_encoding(char *line)\n+static void decode_transfer_encoding(char *line, unsigned linesize)\n {\n \tchar *ep;\n \n \tswitch (transfer_encoding) {\n \tcase TE_QP:\n \t\tep = line + strlen(line);\n-\t\tdecode_q_segment(line, line, ep, 0);\n+\t\tdecode_q_segment(line, line, linesize, ep, 0);\n \t\tbreak;\n \tcase TE_BASE64:\n \t\tep = line + strlen(line);\n-\t\tdecode_b_segment(line, line, ep);\n+\t\tdecode_b_segment(line, line, linesize, ep);\n \t\tbreak;\n \tcase TE_DONTCARE:\n \t\tbreak;\n \t}\n }\n \n-static int handle_filter(char *line);\n+static int handle_filter(char *line, unsigned linesize);\n \n static int find_boundary(void)\n {\n@@ -655,7 +665,7 @@ again:\n \t\t\t\t\t\"can't recover\\n\");\n \t\t\texit(1);\n \t\t}\n-\t\thandle_filter(newline);\n+\t\thandle_filter(newline, sizeof(newline));\n \n \t\t/* skip to the next boundary */\n \t\tif (!find_boundary())\n@@ -670,7 +680,7 @@ again:\n \n \t/* slurp in this section's info */\n \twhile (read_one_header_line(line, sizeof(line), fin))\n-\t\tcheck_header(line, p_hdr_data, 0);\n+\t\tcheck_header(line, sizeof(line), p_hdr_data, 0);\n \n \t/* eat the blank line after section info */\n \treturn (fgets(line, sizeof(line), fin) != NULL);\n@@ -709,9 +719,10 @@ static inline int patchbreak(const char *line)\n }\n \n \n-static int handle_commit_msg(char *line)\n+static int handle_commit_msg(char *line, unsigned linesize)\n {\n \tstatic int still_looking = 1;\n+\tchar *endline = line + linesize;\n \n \tif (!cmitmsg)\n \t\treturn 0;\n@@ -726,13 +737,13 @@ static int handle_commit_msg(char *line)\n \t\t\tif (!*cp)\n \t\t\t\treturn 0;\n \t\t}\n-\t\tif ((still_looking = check_header(cp, s_hdr_data, 0)) != 0)\n+\t\tif ((still_looking = check_header(cp, endline - cp, s_hdr_data, 0)) != 0)\n \t\t\treturn 0;\n \t}\n \n \t/* normalize the log message to UTF-8. */\n \tif (metainfo_charset)\n-\t\tconvert_to_utf8(line, charset);\n+\t\tconvert_to_utf8(line, endline - line, charset);\n \n \tif (patchbreak(line)) {\n \t\tfclose(cmitmsg);\n@@ -751,7 +762,7 @@ static int handle_patch(char *line)\n \treturn 0;\n }\n \n-static int handle_filter(char *line)\n+static int handle_filter(char *line, unsigned linesize)\n {\n \tstatic int filter = 0;\n \n@@ -760,7 +771,7 @@ static int handle_filter(char *line)\n \t */\n \tswitch (filter) {\n \tcase 0:\n-\t\tif (!handle_commit_msg(line))\n+\t\tif (!handle_commit_msg(line, linesize))\n \t\t\tbreak;\n \t\tfilter++;\n \tcase 1:\n@@ -792,14 +803,14 @@ static void handle_body(void)\n \t\t\t/* flush any leftover */\n \t\t\tif ((transfer_encoding == TE_BASE64)  &&\n \t\t\t    (np != newline)) {\n-\t\t\t\thandle_filter(newline);\n+\t\t\t\thandle_filter(newline, sizeof(newline));\n \t\t\t}\n \t\t\tif (!handle_boundary())\n \t\t\t\treturn;\n \t\t}\n \n \t\t/* Unwrap transfer encoding */\n-\t\tdecode_transfer_encoding(line);\n+\t\tdecode_transfer_encoding(line, sizeof(line));\n \n \t\tswitch (transfer_encoding) {\n \t\tcase TE_BASE64:\n@@ -808,7 +819,7 @@ static void handle_body(void)\n \n \t\t\t/* binary data most likely doesn't have newlines */\n \t\t\tif (message_type != TYPE_TEXT) {\n-\t\t\t\trc = handle_filter(line);\n+\t\t\t\trc = handle_filter(line, sizeof(newline));\n \t\t\t\tbreak;\n \t\t\t}\n \n@@ -825,7 +836,7 @@ static void handle_body(void)\n \t\t\t\t\t/* should be sitting on a new line */\n \t\t\t\t\t*(++np) = 0;\n \t\t\t\t\top++;\n-\t\t\t\t\trc = handle_filter(newline);\n+\t\t\t\t\trc = handle_filter(newline, sizeof(newline));\n \t\t\t\t\tnp = newline;\n \t\t\t\t}\n \t\t\t} while (*op != 0);\n@@ -835,7 +846,7 @@ static void handle_body(void)\n \t\t\tbreak;\n \t\t}\n \t\tdefault:\n-\t\t\trc = handle_filter(line);\n+\t\t\trc = handle_filter(line, sizeof(newline));\n \t\t}\n \t\tif (rc)\n \t\t\t/* nothing left to filter */\n@@ -922,7 +933,7 @@ static int mailinfo(FILE *in, FILE *out, int ks, const char *encoding,\n \n \t/* process the email header */\n \twhile (read_one_header_line(line, sizeof(line), fin))\n-\t\tcheck_header(line, p_hdr_data, 1);\n+\t\tcheck_header(line, sizeof(line), p_hdr_data, 1);\n \n \thandle_body();\n \thandle_info();\n-- \n1.5.3.rc7.26.g5f7e4\n"},{"id":"51948","messageId":"6F219888-6F48-4D56-8FA9-BE63EB6E1D95@iki.fi","threadId":"9711","inReplyTo":"3f4fd2640708301435s7067137cp5db6334af844158a@mail.gmail.com","subject":"Re: Buffer overflows","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-08-30T21:51:24Z","receivedAt":"2007-08-30T21:51:24Z","isPatch":false,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On 31.8.2007, at 0.35, Reece Dunn wrote:\n\n>> The problem is that the git code is full of these random cases. It's\n>> simply a huge job to even try to verify the correctness of it. Even\n>> if someone did that and fixed all the problems, tomorrow there would\n>> be new ones because noone bothers to even try to avoid them. So there\n>> really isn't any point in trying to make git secure until the coding\n>> style changes.\n>\n> You don't want a manual check to do these kinds of checks. Not only is\n> it a huge job, you have the human factor: people make mistakes. This\n> is (in part) what the review process is for, but understanding how to\n> identify code that is safe from buffer overruns, integer overflows and\n> the like is a complex task. Also, it may work on 32-bit which has been\n> verified, but not on 64-bit.\n>\n> It would be far better to specify the rules on how to detect these\n> issues into a static analysis tool and have that do the checking for\n> you. Therefore, it is possible to detect when new problems have been\n> added into the codebase. Does sparse support identifying these issues?\n\nYes, it is a complex task. But if there did exist such a static  \nanalyzer tool already, it would probably show that half of the strcpy \n() calls (and others) in git are currently unsafe. Wouldn't help all  \nthat much I think.\n\n>> The code should be easy to verify to be secure, and with some kind of\n>> a safe string API it's a lot easier than trying to figure out corner\n>> cases where strcpy() calls break.\n>\n> Why is it easier? If you have a fixed-size buffer, why not use\n> strncpy, which is what a safe string API is essentially doing anyway?\n\nWell, strncpy() is a pretty good example actually. A lot of people  \nuse it wrong, because they don't realize that it doesn't necessarily  \nNUL-terminate the strings. So it's another example of a bad API that  \ncan be easily used wrong. And besides that, it also fills the rest of  \nthe buffer with NULs, which is almost always pointless waste of CPU.\n\nAnd why is safe string API easier to verify? Here's an example:\n\n// see how easily you can use strncpy() to cause a buffer overflow:\nchar buf[1024];\nstrncpy(buf, input, 2048);\n\n// see how impossible it is to cause a buffer overflow with my static  \nstring API:\nSTR_STATIC(str, 1024);\nsstr_append(str, input);\n\nOf course the above example is a simple one, but often when using  \nlibc string handling functions for building strings the code gets  \ncomplex and there are all kinds of \"is the buffer full already? what  \nabout now? and now? and now?\" and with all of those checks it's easy  \nto make mistakes.\n\nThe point is that if the APIs are (nearly) impossible to use  \ninsecurely, it's very easy to verify that the code is safe. The code  \ndoesn't get safe by lots of checks everywhere, it gets safe by  \nplacing a minimal amount of checks to small area of the code. The  \ncorrectness of a few checks is a lot easier to verify.\n"},{"id":"51951","messageId":"7vtzqg7jrn.fsf@gitster.siamese.dyndns.org","threadId":"9711","inReplyTo":"3f4fd2640708301435s7067137cp5db6334af844158a@mail.gmail.com","subject":"Re: Buffer overflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-30T22:14:04Z","receivedAt":"2007-08-30T22:14:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Reece Dunn\" <msclrhd@googlemail.com> writes:\n\n> Why is it easier? If you have a fixed-size buffer, why not use\n> strncpy, which is what a safe string API is essentially doing anyway?\n\nI would not claim unchecked strcpy is good -- we obviously would\nwant to fix them.\n\nBut at the same time use of strncpy, strlcpy and friends solves\nonly half of the problem.  Often people say \"use strncpy or\nstrlcpy then you would not overstep the buffer\", but that does\nnot really solve anything, without additional logic to deal with\nresulting truncation (barfing with \"insanely long string\" error\nmessage and dying is the least impact).  Continuing the work on\ndata that the user did not intend to give you is just as wrong\nas using corrupt data that overflowed your static buffer.\n\nDoes Timo's nonstandard API solve that issue?  Perhaps it does,\nperhaps not.  Does it make easier to maintain our code?  I\nhighly doubt it in the current shape.\n\nIt is well and widely understood idiom to use strlcpy to a\nfixed-sized buffer and checking the resulting length to make\nsure the result would not have overflowed (and if it would have,\nissue an error and die).  I would not have anything against a\nset of patches to follow such a pattern.\n\nBut a patch to add a non-standard API that nobody else uses,\nwithout any patch to show the changes to a few places that could\nuse the API to demonstrate that the use of API vastly cleans the\ncode up and makes it infinitely harder to make mistakes?\n\nThe API needs to justify itself to convince the people who needs\nto learn and adjust to that the benefit far outweighes deviation\nfrom better known patterns, and I do not see that happening in\nTimo's patch.\n"},{"id":"51953","messageId":"3f4fd2640708301534k40f07a1cva90a59d12ace6138@mail.gmail.com","threadId":"9711","inReplyTo":"6F219888-6F48-4D56-8FA9-BE63EB6E1D95@iki.fi","subject":"Re: Buffer overflows","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2007-08-30T22:34:26Z","receivedAt":"2007-08-30T22:34:26Z","isPatch":false,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"On 30/08/2007, Timo Sirainen <tss@iki.fi> wrote:\n> On 31.8.2007, at 0.35, Reece Dunn wrote:\n>\n> >> The problem is that the git code is full of these random cases. It's\n> >> simply a huge job to even try to verify the correctness of it. Even\n> >> if someone did that and fixed all the problems, tomorrow there would\n> >> be new ones because noone bothers to even try to avoid them. So there\n> >> really isn't any point in trying to make git secure until the coding\n> >> style changes.\n> >\n> > You don't want a manual check to do these kinds of checks. Not only is\n> > it a huge job, you have the human factor: people make mistakes. This\n> > is (in part) what the review process is for, but understanding how to\n> > identify code that is safe from buffer overruns, integer overflows and\n> > the like is a complex task. Also, it may work on 32-bit which has been\n> > verified, but not on 64-bit.\n> >\n> > It would be far better to specify the rules on how to detect these\n> > issues into a static analysis tool and have that do the checking for\n> > you. Therefore, it is possible to detect when new problems have been\n> > added into the codebase. Does sparse support identifying these issues?\n>\n> Yes, it is a complex task. But if there did exist such a static\n> analyzer tool already, it would probably show that half of the strcpy\n> () calls (and others) in git are currently unsafe. Wouldn't help all\n> that much I think.\n\nLet's see:\n\nsymlinks.c(39): In has_symlink_leading_path, the last_symlink input\nargument needs to be large enough to hold MAX_PATH characters, as this\nis sizeof(path). This is a potential risk, depending on how this\nmethod is used.\n\nsideband.c(17): buf is large enough to copy the \"remote:\" string\nliteral into. Also, the buffer is large enough for the input being\nread, as packet_read_line specifies the size of the remaining buffer\nwith an extra character reserved for the null. Therefore, this usage\nis safe.\n\nsha1_file.c(550): open_pack_index does a runtime calculation that\nessentially results in strcpy( \".pack\", \".idx\" ). As these are\nliterals, this usage is safe, provided that strlen(idx_name) >\nstrlen(\".pack\") which should be ensured by the caller. Hence, this\nusage is safe.\n\nSo far, these are looking safe to me. I'd need to check the remaining\nuses to be sure and to find the strcpy usage that you have identified\nas an issue.\n\nIt would also be worth creating a test case that triggers this bug, as\nthat can then be used to ensure that it does not reappear once fixed.\nThat is the best way to deal with these, as not all uses of \"unsafe\"\nAPI are actually unsafe.\n\n> >> The code should be easy to verify to be secure, and with some kind of\n> >> a safe string API it's a lot easier than trying to figure out corner\n> >> cases where strcpy() calls break.\n> >\n> > Why is it easier? If you have a fixed-size buffer, why not use\n> > strncpy, which is what a safe string API is essentially doing anyway?\n>\n> Well, strncpy() is a pretty good example actually. A lot of people\n> use it wrong, because they don't realize that it doesn't necessarily\n> NUL-terminate the strings. So it's another example of a bad API that\n> can be easily used wrong. And besides that, it also fills the rest of\n> the buffer with NULs, which is almost always pointless waste of CPU.\n\nOk, I take your point. However, there is nothing preventing these API\nfrom being used correctly. Granted, this is more work and like the\nstrncpy example, have subtle behaviour, but that does not make it\nimpossible.\n\nAs an example, do your safe API do null pointer checks. This is\nbecause strcpy, strlen and the like don't, which is one of the reasons\nwhy they are considered unsafe. But then, if you guarantee that you\nare not passing a null pointer to one of these API, why take the hit\nof the additional checks when you know that these are safe. It is also\neasier to debug the problem if it happens at the point of call,\ninstead of silently working.\n\nOne of the problems with a safe string API is that there are different\npossible things to do when an overrun is detected. The best thing to\ndo would be to cap the input to the buffer size and return some error\nstatus. For example, you could have:\n\n    STR_STATIC(str, 1);\n    sstr_append(str, \"/foo/bar\");\n    // run \"rm -rf ${str}\"\n\nwhich would hose your system. This is worse than a buffer overrun.\n\n> And why is safe string API easier to verify? Here's an example:\n>\n> // see how easily you can use strncpy() to cause a buffer overflow:\n> char buf[1024];\n> strncpy(buf, input, 2048);\n\nYes it can, as above. A subtler case would be when the space for a\nnull terminator is missed. But then I'd write something like:\n\n    char buf[1024];\n    strncpy(buf, input, sizeof(buf));\n\nHere, the compiler does the work for me, so if I change 1024 to 512,\nit will still be safe.\n\n> // see how impossible it is to cause a buffer overflow with my static\n> string API:\n> STR_STATIC(str, 1024);\n> sstr_append(str, input);\n\nHow is str defined? Looking at the code, I can't _see_ that this is\nsafe. All I see as inputs are `str` and `input`, so how can I verify\nthat this is correct without looking at how these are defined.\n\n> Of course the above example is a simple one, but often when using\n> libc string handling functions for building strings the code gets\n> complex and there are all kinds of \"is the buffer full already? what\n> about now? and now? and now?\" and with all of those checks it's easy\n> to make mistakes.\n\nBut also, those API are well known, along with their associated\nproblems. By creating a new API, there are new (unknown) ways to\nabuse/misuse it.\n\n> The point is that if the APIs are (nearly) impossible to use\n> insecurely, it's very easy to verify that the code is safe. The code\n> doesn't get safe by lots of checks everywhere, it gets safe by\n> placing a minimal amount of checks to small area of the code. The\n> correctness of a few checks is a lot easier to verify.\n\nBut what about that nearly impossible case?\n\nAlso, how do you use the safe string API with other API that don't\nhave a \"safe\" equivalent?\n\n- Reece\n"},{"id":"51954","messageId":"20070830223616.GA29200@artemis.corp","threadId":"9711","inReplyTo":"7vtzqg7jrn.fsf@gitster.siamese.dyndns.org","subject":"Re: Buffer overflows","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-08-30T22:36:16Z","receivedAt":"2007-08-30T22:36:16Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Aug 30, 2007 at 10:14:04PM +0000, Junio C Hamano wrote:\n> \"Reece Dunn\" <msclrhd@googlemail.com> writes:\n> \n> > Why is it easier? If you have a fixed-size buffer, why not use\n> > strncpy, which is what a safe string API is essentially doing anyway?\n> \n> I would not claim unchecked strcpy is good -- we obviously would\n> want to fix them.\n> \n> But at the same time use of strncpy, strlcpy and friends solves\n> only half of the problem.\n\n  Actually, strncpy solves nothing as it's completely broken in so many\nways: it does not necessarily ends the string with a NUL-char, and it\nNUL-pads the buffer, making it really slow when you use it top copy 10\nchars in a BUFSIZ-big buffer.\n\n  strncpy should never ever be used, few programmers understand it, and\nit's very error prone.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"51955","messageId":"16CA3709-5E04-4CEA-B154-2339C5E4D23C@iki.fi","threadId":"9711","inReplyTo":"7vtzqg7jrn.fsf@gitster.siamese.dyndns.org","subject":"Re: Buffer overflows","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-08-30T22:41:03Z","receivedAt":"2007-08-30T22:41:03Z","isPatch":false,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On 31.8.2007, at 1.14, Junio C Hamano wrote:\n\n> \"Reece Dunn\" <msclrhd@googlemail.com> writes:\n>\n>> Why is it easier? If you have a fixed-size buffer, why not use\n>> strncpy, which is what a safe string API is essentially doing anyway?\n>\n> I would not claim unchecked strcpy is good -- we obviously would\n> want to fix them.\n>\n> But at the same time use of strncpy, strlcpy and friends solves\n> only half of the problem.  Often people say \"use strncpy or\n> strlcpy then you would not overstep the buffer\", but that does\n> not really solve anything, without additional logic to deal with\n> resulting truncation (barfing with \"insanely long string\" error\n> message and dying is the least impact).  Continuing the work on\n> data that the user did not intend to give you is just as wrong\n> as using corrupt data that overflowed your static buffer.\n\nSure, I agree with that. In my own code I avoid static buffer sizes  \nas much as I can. But since git's code is far from even trying to do  \nthat, I thought it would be easier to start with a simpler plan: Try  \nnot to have remote code execution holes that can be found with 2  \nminutes of looking at the code.\n\n> Does Timo's nonstandard API solve that issue?  Perhaps it does,\n> perhaps not.\n\nThat would require moving to dynamically growing strings, which in  \nturn requires freeing the strings afterwards so it's not such a  \nsimple job. Simply replacing the current char[] buffers with my  \nstatic_string would be quick and easy and although it wouldn't fix  \nall potential problems, it would fix most of the buffer overflows.\n\n> Does it make easier to maintain our code?  I highly doubt it in the  \n> current shape.\n\nDepends on what you mean by \"maintain\". I find the resulting code a  \nlot easier to understand, and a lot easier to verify for correctness  \nand safety. Here's an example: http://marc.info/? \nl=git&m=117962988914013&w=2\n\nBut then again you and Alex didn't seem to think so.\n\n> It is well and widely understood idiom to use strlcpy to a\n> fixed-sized buffer and checking the resulting length to make\n> sure the result would not have overflowed (and if it would have,\n> issue an error and die).  I would not have anything against a\n> set of patches to follow such a pattern.\n\nI don't like strlcpy()/strlcat() all that much either, because  \nchecking the overflow is more difficult than it needs to be. For  \nexample:\n\nif (strlcpy(dest, src, sizeof(dest)) <= sizeof(dest)) overflow();\n// compared to a function that simply returns if it overflowed or not:\nif (strocpy(dest, src, sizeof(dest)) < 0) overflow();\n\nActually I'm not even sure if the above strlcpy() check is right. Is  \nit <= or <?\n\n> The API needs to justify itself to convince the people who needs\n> to learn and adjust to that the benefit far outweighes deviation\n> from better known patterns, and I do not see that happening in\n> Timo's patch.\n\nThe better known patterns are being used insecurely all the time now,  \nso I can't really see how this would be anything worse.\n\nAnyway my point wasn't to get my code into git. I just wanted that  \n*something* would be done about this. Currently I just can't see  \nmyself wanting to use git, because it limits what I can do with it.  \nAny kind of automated processing is completely out of the question  \nbecause then attackers could easily take over my machine if they  \nwanted to.\n"},{"id":"51957","messageId":"7vir6w7hyl.fsf@gitster.siamese.dyndns.org","threadId":"9711","inReplyTo":"20070830214824.GC15405@steel.home","subject":"Re: [PATCH] Temporary fix for stack smashing in mailinfo","fromName":"Junio C Hamano","fromEmail":"junkio@pobox.com","sentAt":"2007-08-30T22:53:06Z","receivedAt":"2007-08-30T22:53:06Z","isPatch":true,"sender":{"key":"junkio@pobox.com","avatar":null},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Junio, I cannot have time to fix the code nice and proper, but as\n> heavy user of git-am just have to have it fixed at least a like this.\n> And this is ugly (and definitely incomplete), everyone be warned.\n>\n> Checked with valgrind, looks good (except for iconv_open reading past\n> one of its arguments):\n\nOn the top of your patch, I think decode_header_bq() needs to\nmake sure that a string with more than one pieces, each of which\ndecodes well within piecebuf, cannot overflow outbuf[] in the\nwhile loop.\n\n> @@ -578,56 +588,56 @@ static int decode_header_bq(char *it)\n>  \t\tdefault:\n>  \t\t\treturn rfc2047; /* no munging */\n>  \t\tcase 'b':\n> -\t\t\tsz = decode_b_segment(cp + 3, piecebuf, ep);\n> +\t\t\tsz = decode_b_segment(cp + 3, piecebuf, sizeof(piecebuf), ep);\n>  \t\t\tbreak;\n>  \t\tcase 'q':\n> -\t\t\tsz = decode_q_segment(cp + 3, piecebuf, ep, 1);\n> +\t\t\tsz = decode_q_segment(cp + 3, piecebuf, sizeof(piecebuf), ep, 1);\n>  \t\t\tbreak;\n>  \t\t}\n>  \t\tif (sz < 0)\n>  \t\t\treturn rfc2047;\n>  \t\tif (metainfo_charset)\n> -\t\t\tconvert_to_utf8(piecebuf, charset_q);\n> +\t\t\tconvert_to_utf8(piecebuf, sizeof(piecebuf), charset_q);\n>  \t\tstrcpy(out, piecebuf);\n>  \t\tout += strlen(out);\n>  \t\tin = ep + 2;\n>  \t}\n\nIt might also make sense to redo the lower level decoding\nfunctions using existing strbuf interface to build string\nwithout pre-set bounds.\n"},{"id":"51980","messageId":"alpine.LFD.0.999.0708302050170.25853@woody.linux-foundation.org","threadId":"9711","inReplyTo":"7D84F3C7-129D-4197-AAF1-46298E5D0136@iki.fi","subject":"Re: Buffer overflows","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-31T04:09:16Z","receivedAt":"2007-08-31T04:09:16Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 31 Aug 2007, Timo Sirainen wrote:\n> > \n> > Perhaps because your patch was using a totally nonstandard and slow\n> > interface, and had nasty string declaration issues, as people even pointed\n> > out to you.\n> \n> Slow?\n\nHaving a string library, and then implementing \"str_append()\" with a \nstrlen() sounds pretty disgusting to me. \n\nGcc could have optimized the strlen() away for constant string arguments, \nbut since you made the thing out-of-line, it can't do that any more.\n\nSo yes, I bet there are faster string libraries out there.\n\n> The code should be easy to verify to be secure, and with some kind of a safe\n> string API it's a lot easier than trying to figure out corner cases where\n> strcpy() calls break.\n\nI actually looked at the patches, and the \"stringbuf()\" thing was just too \nugly to live. It was also nonportable, in that you use the reserved \nnamespace (which we do extensively in the kernel, but that's an \n\"embdedded\" application that doesn't use system header files).\n\nSo the API was anything but \"safe\".\n\nI think something like that could work, but it really should be done \nright, or not at all.\n\n\t\tLinus\n"},{"id":"51983","messageId":"1188536430.29782.903.camel@hurina","threadId":"9711","inReplyTo":"alpine.LFD.0.999.0708302050170.25853@woody.linux-foundation.org","subject":"Re: Buffer overflows","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-08-31T05:00:30Z","receivedAt":"2007-08-31T05:00:30Z","isPatch":false,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On Thu, 2007-08-30 at 21:09 -0700, Linus Torvalds wrote:\n> \n> On Fri, 31 Aug 2007, Timo Sirainen wrote:\n> > > \n> > > Perhaps because your patch was using a totally nonstandard and slow\n> > > interface, and had nasty string declaration issues, as people even pointed\n> > > out to you.\n> > \n> > Slow?\n> \n> Having a string library, and then implementing \"str_append()\" with a \n> strlen() sounds pretty disgusting to me. \n> \n> Gcc could have optimized the strlen() away for constant string arguments, \n> but since you made the thing out-of-line, it can't do that any more.\n> \n> So yes, I bet there are faster string libraries out there.\n\nOh, well that's easy to fix. But I don't think the speed matters much in\nstring manipulation, it's usually not done in performance critical\npaths.\n\n> > The code should be easy to verify to be secure, and with some kind of a safe\n> > string API it's a lot easier than trying to figure out corner cases where\n> > strcpy() calls break.\n> \n> I actually looked at the patches, and the \"stringbuf()\" thing was just too \n> ugly to live. It was also nonportable, in that you use the reserved \n> namespace (which we do extensively in the kernel, but that's an \n> \"embdedded\" application that doesn't use system header files).\n> \n> So the API was anything but \"safe\".\n\nSome of those were fixed in the patch I sent at the beginning of this\nthread. But since you asked, attached is yet another version of it. As\nfar as I know it's fully C99 compatible. Would be C89 but there's that\nva_copy() call.\n\nI guess it could still be optimized more, but at least strlen() is now\nin a macro. :)\n\nOr if you don't like the STR_STATIC() macro, another way would be:\n\n#define STR_STATIC_INIT(buf) { buf, sizeof(buf), 0, }\n\nchar static_buf[1024];\nstruct string str = STR_STATIC_INIT(static_buf);\n\nstr_append(&str, \"hello world\");\n\n\n\n--- /dev/null\t2007-07-12 03:40:41.165212167 +0300\n+++ str.h\t2007-08-31 07:35:04.000000000 +0300\n@@ -0,0 +1,33 @@\n+#ifndef STR_H\n+#define STR_H\n+\n+struct string {\n+\tchar *buf;\n+\n+\tunsigned int alloc_size;\n+\tunsigned int len;\n+\n+\tunsigned int malloced:1;\n+\tunsigned int overflowed:1;\n+};\n+\n+#define STR_STATIC(name, size) \\\n+\tstruct { \\\n+\t  struct string str; \\\n+\t  char string_buf[(size) + 1]; \\\n+\t} name = { { (name).string_buf, sizeof((name).string_buf), 0, } }\n+\n+extern struct string *str_alloc(unsigned int initial_size);\n+extern void str_free(struct string **str);\n+\n+extern void str_append_n(struct string *str, const char *cstr,\n+\t\t\t unsigned int len);\n+extern void str_printfa(struct string *str, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+extern void str_truncate(struct string *str, unsigned int len);\n+\n+#define str_append(str, cstr) str_append_n(str, cstr, strlen(cstr))\n+#define str_c(str) ((str)->buf)\n+#define str_len(str) ((str)->len)\n+\n+#endif\n--- /dev/null\t2007-07-12 03:40:41.165212167 +0300\n+++ str.c\t2007-08-31 07:36:56.000000000 +0300\n@@ -0,0 +1,85 @@\n+#include \"str.h\"\n+#include \"git-compat-util.h\"\n+\n+struct string *str_alloc(unsigned int initial_size)\n+{\n+\tstruct string *str;\n+\n+\tstr = xcalloc(sizeof(*str), 1);\n+\tstr->alloc_size = initial_size + 1;\n+\tstr->buf = xmalloc(str->alloc_size);\n+\tstr->buf[0] = '\\0';\n+\treturn str;\n+}\n+\n+void str_free(struct string **_str)\n+{\n+\tstruct string *str = *_str;\n+\n+\tif (str->malloced)\n+\t\tfree(str->buf);\n+\tstr->buf = NULL;\n+\tfree(str);\n+\n+\t*_str = NULL;\n+}\n+\n+static void str_grow_if_needed(struct string *str, unsigned int *len)\n+{\n+\tunsigned int avail = str->alloc_size - str->len;\n+\n+\tif (*len >= avail) {\n+\t\tif (!str->malloced) {\n+\t\t\t/* static buffer size */\n+\t\t\t*len = avail;\n+\t\t\tstr->overflowed = 1;\n+\t\t} else {\n+\t\t\tstr->alloc_size = (str->len + *len) * 2;\n+\t\t\tstr->buf = xrealloc(str->buf, str->alloc_size);\n+\t\t}\n+\t}\n+}\n+\n+void str_append_n(struct string *str, const char *cstr, unsigned int len)\n+{\n+\tstr_grow_if_needed(str, &len);\n+\tmemcpy(str->buf + str->len, cstr, len);\n+\tstr->len += len;\n+\tstr->buf[str->len] = '\\0';\n+}\n+\n+void str_printfa(struct string *str, const char *fmt, ...)\n+{\n+\tunsigned int len, avail = str->alloc_size - str->len;\n+\tva_list va, va2;\n+\tint ret;\n+\n+\tva_start(va, fmt);\n+\tva_copy(va2, va);\n+\tret = vsnprintf(str->buf + str->len, avail, fmt, va);\n+\tassert(ret >= 0);\n+\n+\tif ((unsigned int)ret >= avail) {\n+\t\tif (!str->malloced) {\n+\t\t\tstr->overflowed = 1;\n+\t\t\tret = avail == 0 ? 0 : avail-1;\n+\t\t} else {\n+\t\t\tlen = ret;\n+\t\t\tstr_grow_if_needed(str, &len);\n+\t\t\tavail = str->alloc_size - str->len;\n+\n+\t\t\tret = vsnprintf(str->buf + str->len, avail, fmt, va2);\n+\t\t\tassert(ret >= 0 && (unsigned int)ret < avail);\n+\t\t}\n+\t}\n+\tstr->len += ret;\n+\tva_end(va);\n+}\n+\n+void str_truncate(struct string *str, unsigned int len)\n+{\n+\tif (len >= str->alloc_size)\n+\t\tlen = str->alloc_size - 1;\n+\tstr->len = len;\n+\tstr->buf[len] = '\\0';\n+}\n"},{"id":"52015","messageId":"46D7E512.2010905@op5.se","threadId":"9711","inReplyTo":"1188536430.29782.903.camel@hurina","subject":"Re: Buffer overflows","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2007-08-31T09:53:22Z","receivedAt":"2007-08-31T09:53:22Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Timo Sirainen wrote:\n> On Thu, 2007-08-30 at 21:09 -0700, Linus Torvalds wrote:\n>> On Fri, 31 Aug 2007, Timo Sirainen wrote:\n>>>> Perhaps because your patch was using a totally nonstandard and slow\n>>>> interface, and had nasty string declaration issues, as people even pointed\n>>>> out to you.\n>>> Slow?\n>> Having a string library, and then implementing \"str_append()\" with a \n>> strlen() sounds pretty disgusting to me. \n>>\n>> Gcc could have optimized the strlen() away for constant string arguments, \n>> but since you made the thing out-of-line, it can't do that any more.\n>>\n>> So yes, I bet there are faster string libraries out there.\n> \n> Oh, well that's easy to fix. But I don't think the speed matters much in\n> string manipulation, it's usually not done in performance critical\n> paths.\n> \n\nIn git it is, as strings are manipulated en masse in order to traverse\nhistory, look up paths from index/filesystem etc, etc.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n"},{"id":"52017","messageId":"Pine.LNX.4.64.0708311104200.28586@racer.site","threadId":"9711","inReplyTo":"1188536430.29782.903.camel@hurina","subject":"Re: Buffer overflows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-31T10:06:01Z","receivedAt":"2007-08-31T10:06:01Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 31 Aug 2007, Timo Sirainen wrote:\n\n> On Thu, 2007-08-30 at 21:09 -0700, Linus Torvalds wrote:\n> \n> > So yes, I bet there are faster string libraries out there.\n> \n> Oh, well that's easy to fix. But I don't think the speed matters much in \n> string manipulation, it's usually not done in performance critical \n> paths.\n\nAFAIR one of the recent performance studies showed strlen() as one of the \n_biggest_ offenders.  So no, your point has been disproven _already_.\n\nI have to wonder, though, why you do not just go and enhance strbuf.[ch], \nwhich is nice and easy, and not the least unelegant.\n\nCiao,\nDscho\n"},{"id":"52019","messageId":"BD9F3FD0-94EF-4182-A03B-B26B18544894@wincent.com","threadId":"9711","inReplyTo":"3f4fd2640708301534k40f07a1cva90a59d12ace6138@mail.gmail.com","subject":"Re: Buffer overflows","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-08-31T10:52:42Z","receivedAt":"2007-08-31T10:52:42Z","isPatch":false,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 31/8/2007, a las 0:34, Reece Dunn escribió:\n\n> As an example, do your safe API do null pointer checks. This is\n> because strcpy, strlen and the like don't, which is one of the reasons\n> why they are considered unsafe. But then, if you guarantee that you\n> are not passing a null pointer to one of these API, why take the hit\n> of the additional checks when you know that these are safe.\n\nDo you really think that comparing a pointer to NULL is going to be a  \nspeed hit? I would imagine that on most architectures it boils down  \nto one or two machine code instructions.\n\nCheers,\nWincent\n"},{"id":"52036","messageId":"46D80E39.8060106@fs.ei.tum.de","threadId":"9711","inReplyTo":"BD9F3FD0-94EF-4182-A03B-B26B18544894@wincent.com","subject":"Re: Buffer overflows","fromName":"Simon 'corecode' Schubert","fromEmail":"corecode@fs.ei.tum.de","sentAt":"2007-08-31T12:48:57Z","receivedAt":"2007-08-31T12:48:57Z","isPatch":false,"sender":{"key":"corecode@fs.ei.tum.de","avatar":"https://gravatar.com/avatar/eff9dbf0cdac0d1e6a6cd7ed0e50763edcb376b493b5253a35ff167918ad79e1?d=mp&s=160"},"body":"Wincent Colaiuta wrote:\n>> As an example, do your safe API do null pointer checks. This is\n>> because strcpy, strlen and the like don't, which is one of the reasons\n>> why they are considered unsafe. But then, if you guarantee that you\n>> are not passing a null pointer to one of these API, why take the hit\n>> of the additional checks when you know that these are safe.\n> Do you really think that comparing a pointer to NULL is going to be a \n> speed hit? I would imagine that on most architectures it boils down to \n> one or two machine code instructions.\n\nThe question rather is, why should you bother comparing to a NULL pointer?  To return an error (EINVAL?)?  I'd rather have either a) the caller check or b) the process segfault.  A segfault gives me a nice core file which I can use to hunt the bug.\n\nI also don't see why not checking for NULL pointers is unsafe.  Okay, maybe there are platforms out there which do not crash on a NULL pointer derefence, but I doubt these are consumers of git.  All other platforms are safe by the implicit check of the MMU.\n\nThe worst thing is something like\n\nif (ptr == NULL)\n\tabort();\n\nwhich only adds code (and thus needs maintenance), but no value whatsoever.  Either the following code tolerates NULL pointers or it will crash and segfault, so why bother panicing before.\n\nOf course I might be totally of track...\n\ncheers\n  simon\n"},{"id":"52196","messageId":"200709021542.31100.johan@herland.net","threadId":"9711","inReplyTo":"7vtzqg7jrn.fsf@gitster.siamese.dyndns.org","subject":"Re: Buffer overflows","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2007-09-02T13:42:30Z","receivedAt":"2007-09-02T13:42:30Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Friday 31 August 2007, Junio C Hamano wrote:\n> It is well and widely understood idiom to use strlcpy to a\n> fixed-sized buffer and checking the resulting length to make\n> sure the result would not have overflowed (and if it would have,\n> issue an error and die).  I would not have anything against a\n> set of patches to follow such a pattern.\n> \n> But a patch to add a non-standard API that nobody else uses,\n> without any patch to show the changes to a few places that could\n> use the API to demonstrate that the use of API vastly cleans the\n> code up and makes it infinitely harder to make mistakes?\n> \n> The API needs to justify itself to convince the people who needs\n> to learn and adjust to that the benefit far outweighes deviation\n> from better known patterns, and I do not see that happening in\n> Timo's patch.\n\nSo in general, git people seem to be saying that:\n\n1. Yes, we agree that the C string library suX0rs badly.\n\n2. There are more than 0 string manipulation bugs (e.g. buffer overflows) in \ngit. The number may be small or large, but I have yet to see anyone claim \nit's _zero_.\n\n3. Timo's patches (in their current form) are not the way to go, because of \nnon-standard API, implementation problems, whatever...\n\nSo why does the discussion end there? Lukas proposed an interesting \nalternative in \"The Better String Library\" ( \nhttp://bstring.sourceforge.net/ ). Why has there been lots of bashing on \nTimo's efforts, but no critique of bstring? I'd be very keen to know what \nthe git developers think of it. AFAICS, it seems to fulfill at least _some_ \nof the problems people find in Timo's patches. Specifically, it claims:\n\n- High performance (better than the C string library)\n- Simple usage\n\nI'd also say it's probably more widely used than Timo's patches.\n\n\nIf the only response to Timo's highlighting of string manipulation problems \nin git, is for us to flame his patches and leave it at that, then I have no \nchoice but to agree with him in that security does not seem to matter to \nus.\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"52208","messageId":"3f4fd2640709020811r4ea8f01fw775257859e26af29@mail.gmail.com","threadId":"9711","inReplyTo":"200709021542.31100.johan@herland.net","subject":"Re: Buffer overflows","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2007-09-02T15:11:32Z","receivedAt":"2007-09-02T15:11:32Z","isPatch":false,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"On 02/09/07, Johan Herland <johan@herland.net> wrote:\n> On Friday 31 August 2007, Junio C Hamano wrote:\n> > The API needs to justify itself to convince the people who needs\n> > to learn and adjust to that the benefit far outweighes deviation\n> > from better known patterns, and I do not see that happening in\n> > Timo's patch.\n>\n> So in general, git people seem to be saying that:\n>\n> 1. Yes, we agree that the C string library suX0rs badly.\n>\n> 2. There are more than 0 string manipulation bugs (e.g. buffer overflows) in\n> git. The number may be small or large, but I have yet to see anyone claim\n> it's _zero_.\n>\n> 3. Timo's patches (in their current form) are not the way to go, because of\n> non-standard API, implementation problems, whatever...\n>\n> So why does the discussion end there? Lukas proposed an interesting\n> alternative in \"The Better String Library\" (\n> http://bstring.sourceforge.net/ ). Why has there been lots of bashing on\n> Timo's efforts, but no critique of bstring? I'd be very keen to know what\n> the git developers think of it. AFAICS, it seems to fulfill at least _some_\n> of the problems people find in Timo's patches. Specifically, it claims:\n>\n> - High performance (better than the C string library)\n> - Simple usage\n\nPerforming a brief look at the documentation, the bstring library\nlooks promising.\n\nIt looks like it has an allocate and grow internal buffers on demand\npolicy. This is similar to what the C++ std::basic_string does, as\nwell as the string helpers in the Boost version of Jam (written in C).\nThis hides the buffer management from the user of the library, rather\nthan obfuscating it like in Timo's patch.\n\nThe API defined in the documentation is well thought out and\nextensive, moreso than in the efforts by Timo and others. It has the\ntraditional C API, along with other API found in other string\nlibraries (such as split and join). I am not sure how much of git\ncould make use of these, but they have the pontential to simplify some\nareas of the codebase.\n\nLooking at the documentation, it is clear that this is a well thought\nout library, both from the problems/security issues of the C library\nand to how it compares with other string libraries. As well as\ncovering buffer overflow, it also deals with things like integer\noverflow.\n\nThey have also done performance tests comparing the bstring library to\nthe C API and C++ std::string. With the C API comparison, the library\nperforms about 10% slower for string assignment, but other areas don't\nhave a slowdown. In fact, string concatenation is _considerably_\nimproved, something that will help git performance. I suspect (but\nhave not verified) that the slowdown on assignment is due to buffer\nallocation.\n\n> I'd also say it's probably more widely used than Timo's patches.\n\nWhich is good, as this means that along with the tests in the library,\nit will be more stable and less likely to be buggy than something that\nis written from scratch.\n\n> If the only response to Timo's highlighting of string manipulation problems\n> in git, is for us to flame his patches and leave it at that, then I have no\n> choice but to agree with him in that security does not seem to matter to\n> us.\n\nI would not like to see that happen. It seems that the bstring library\nwill help git in more ways than security, by improving string\nconcatenation performance and giving a richer string API without\nsacrificing performance (except where noted) and code clarity.\n\nIt would be interesting to see how the 10% performance drop on string\nassignment impacts git performance, when balanced with the drastic\n(92x in the performance table) increase on string concatenation.\n\nThe only major issue that I can see with bstring is that it does not\nhave a wchar_t version, but git is using chars internally, so this is\nnot a problem for git.\n\n- Reece\n"},{"id":"52210","messageId":"85veatqelm.fsf@lola.goethe.zz","threadId":"9711","inReplyTo":"3f4fd2640709020811r4ea8f01fw775257859e26af29@mail.gmail.com","subject":"Re: Buffer overflows","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-09-02T15:19:49Z","receivedAt":"2007-09-02T15:19:49Z","isPatch":false,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"\"Reece Dunn\" <msclrhd@googlemail.com> writes:\n\n> Which is good, as this means that along with the tests in the\n> library, it will be more stable and less likely to be buggy than\n> something that is written from scratch.\n\nRemember git's history.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"52214","messageId":"3f4fd2640709020835w4058c55dqb753613bde7aa533@mail.gmail.com","threadId":"9711","inReplyTo":"85veatqelm.fsf@lola.goethe.zz","subject":"Re: Buffer overflows","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2007-09-02T15:35:32Z","receivedAt":"2007-09-02T15:35:32Z","isPatch":false,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"On 02/09/07, David Kastrup <dak@gnu.org> wrote:\n> \"Reece Dunn\" <msclrhd@googlemail.com> writes:\n>\n> > Which is good, as this means that along with the tests in the\n> > library, it will be more stable and less likely to be buggy than\n> > something that is written from scratch.\n>\n> Remember git's history.\n\nTrue!\n\n- Reece\n"},{"id":"52240","messageId":"46DAF039.2000208@lsrfire.ath.cx","threadId":"9711","inReplyTo":"200709021542.31100.johan@herland.net","subject":"Re: Buffer overflows","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-09-02T17:17:45Z","receivedAt":"2007-09-02T17:17:45Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Johan Herland schrieb:\n> So why does the discussion end there? Lukas proposed an interesting \n> alternative in \"The Better String Library\" ( \n> http://bstring.sourceforge.net/ ). Why has there been lots of bashing on \n> Timo's efforts, but no critique of bstring? I'd be very keen to know what \n> the git developers think of it. AFAICS, it seems to fulfill at least _some_ \n> of the problems people find in Timo's patches. Specifically, it claims:\n> \n> - High performance (better than the C string library)\n> - Simple usage\n> \n> I'd also say it's probably more widely used than Timo's patches.\n> \n> \n> If the only response to Timo's highlighting of string manipulation problems \n> in git, is for us to flame his patches and leave it at that, then I have no \n> choice but to agree with him in that security does not seem to matter to \n> us.\n\nWell, a patch (8dabdfcc) from Alex Riesen has made it into 1.5.3 which\nfixes some of the problems.  That's a start.\n\nAnd don't forget that we have our very own string library, viz.\nstrbuf.c, which could see more use.\n\nThat said, I agree that bstring looks well thought out.  It's also quite\nlarge (lots of functions, lots of code where a bug might lurk).  Hmm.\n\nNow if only someone could demonstrate the advantages of using bstring in\ngit by posting a nice patch.. :-P\n\nRené\n"},{"id":"52242","messageId":"46DAF550.70300@etek.chalmers.se","threadId":"9711","inReplyTo":"46DAF039.2000208@lsrfire.ath.cx","subject":"Re: Buffer overflows","fromName":"Lukas Sandström","fromEmail":"lukass@etek.chalmers.se","sentAt":"2007-09-02T17:39:28Z","receivedAt":"2007-09-02T17:39:28Z","isPatch":false,"sender":{"key":"luksan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/152281?v=4"},"body":"René Scharfe wrote:\n> And don't forget that we have our very own string library, viz.\n> strbuf.c, which could see more use.\n> \n> That said, I agree that bstring looks well thought out.  It's also quite\n> large (lots of functions, lots of code where a bug might lurk).  Hmm.\n> \n> Now if only someone could demonstrate the advantages of using bstring in\n> git by posting a nice patch.. :-P\n> \n> René\n\nI'm currently working on rewriting builtin-mailinfo.c to use bstring. \nI'll hopefully have a proof-of-concept ready today or tomorrow.\n\n/Lukas\n"},{"id":"52285","messageId":"fbfju8$7gh$2@sea.gmane.org","threadId":"9711","inReplyTo":"85veatqelm.fsf@lola.goethe.zz","subject":"Re: Buffer overflows","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-09-03T00:19:24Z","receivedAt":"2007-09-03T00:19:24Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"David Kastrup wrote:\n\n> \"Reece Dunn\" <msclrhd@googlemail.com> writes:\n> \n>> Which is good, as this means that along with the tests in the\n>> library, it will be more stable and less likely to be buggy than\n>> something that is written from scratch.\n> \n> Remember git's history.\n\nOn the other hand instead of doing binary diffs from a scratch, git uses\n[customized] LibXDiff library. The same might be done with bstring library,\nI think.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"52288","messageId":"7v642soai5.fsf@gitster.siamese.dyndns.org","threadId":"9711","inReplyTo":"fbfju8$7gh$2@sea.gmane.org","subject":"Re: Buffer overflows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-03T00:31:14Z","receivedAt":"2007-09-03T00:31:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> David Kastrup wrote:\n> ...\n>> Remember git's history.\n>\n> On the other hand instead of doing binary diffs from a scratch, git uses\n> [customized] LibXDiff library. The same might be done with bstring library,\n> I think.\n\nI am very much in favor of reusing code that aleady proved\nitself in the field outside git's context.\n"}]}