{"thread":{"id":"8229","subject":"[PATCH 1/3] Added generic string handling code.","startedAt":"2007-05-20T02:24:29Z","lastAt":"2007-05-23T13:56:50Z","messageCount":7,"participants":["Timo Sirainen","Alex Riesen","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"42666","messageId":"1179627869.32181.1284.camel@hurina","threadId":"8229","inReplyTo":null,"subject":"[PATCH 1/3] Added generic string handling code.","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-05-20T02:24:29Z","receivedAt":"2007-05-20T02:24:29Z","isPatch":true,"sender":{"key":"tss@iki.fi","avatar":null},"body":"Aren't you already tired of using the crappy string handling functions\nthat libc provides? I see a lot of really ugly code in git that exists\njust because this.\n\nI also see a lot of potential buffer overflows because either no\noverflow checking is done, or it's done wrong. Perhaps it doesn't matter\nnow if you're manually inspecting each patch before feeding to git, but\nI fear that in future someone's automated git handler will be\nresponsible for getting malicious code added into Linux, just because of\na simple buffer overflow that could have been easily avoided.\n\nSo here's my try on starting with something simple. Unlike almost all\nother string handling libraries, it doesn't allocate the memory\ndynamically. This makes it really easy to convert existing code to use\nit. I'm including some example changes in the other patches. Besides\nmaking the code safer, it can also make it faster, especially those\nstrcat() replacements.\n\nI'm aware of strbuf.[ch], but I wasn't sure if I should have merged this\ncode with it or what. It had this \"eof\" field which I think makes it\nmore like a \"file reader string\" and not a \"string buffer\". So I just\nadded new str.[ch] files.\n\n---\n Makefile |    4 ++--\n str.c    |   40 ++++++++++++++++++++++++++++++++++++++++\n str.h    |   32 ++++++++++++++++++++++++++++++++\n 3 files changed, 74 insertions(+), 2 deletions(-)\n create mode 100644 str.c\n create mode 100644 str.h\n\ndiff --git a/Makefile b/Makefile\nindex 29243c6..f61ad50 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -294,7 +294,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 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 mailmap.h\n \n@@ -312,7 +312,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 \\\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.c b/str.c\nnew file mode 100644\nindex 0000000..d46e7f4\n--- /dev/null\n+++ b/str.c\n@@ -0,0 +1,40 @@\n+#include \"str.h\"\n+\n+void _str_append(struct string *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_printfa(struct string *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 < 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_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..99d3215\n--- /dev/null\n+++ b/str.h\n@@ -0,0 +1,32 @@\n+#ifndef STR_H\n+#define STR_H\n+\n+#include \"git-compat-util.h\"\n+\n+struct string {\n+\tunsigned int size;\n+\tunsigned int len:31;\n+\tunsigned int overflowed:1;\n+\tchar buf[];\n+};\n+\n+#define stringbuf(name, size) \\\n+\tunion { \\\n+\t  struct string string; \\\n+\t  char string_buf[sizeof(struct string) + (size) + 1]; \\\n+\t} name = { { (size)+1, 0, 0 } }\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_append(str, cstr) _str_append(&(str).string, cstr)\n+#define str_printfa(str, fmt, ...) _str_printfa(&(str).string, fmt, __VA_ARGS__)\n+#define str_truncate(str, len) _str_truncate(&(str).string, len)\n+\n+#define str_c(str) ((str).string.buf)\n+#define str_len(str) ((str).string.len)\n+#define str_overflowed(str) ((str).string.overflowed)\n+\n+#endif\n-- \n1.5.1.4\n\n\n"},{"id":"42685","messageId":"20070520100155.GB3106@steel.home","threadId":"8229","inReplyTo":"1179627869.32181.1284.camel@hurina","subject":"Re: [PATCH 1/3] Added generic string handling code.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-20T10:01:55Z","receivedAt":"2007-05-20T10:01:55Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Timo Sirainen, Sun, May 20, 2007 04:24:29 +0200:\n> So here's my try on starting with something simple. Unlike almost all\n> other string handling libraries, it doesn't allocate the memory\n> dynamically.\n\nSometimes you _need_ dinamic memory allocation.\n\n> This makes it really easy to convert existing code to use it. I'm\n> including some example changes in the other patches. Besides making\n> the code safer, it can also make it faster, especially those\n> strcat() replacements.\n\nIt is also bigger, heavier on stack and sometimes slower because of\nmore function calls involved.\n\nAside from that, I like it. I wouldn't use it universally, but\nthere were times when I wished it has been be done this way.\n"},{"id":"42690","messageId":"B5F56D1C-4EC2-4A79-9489-B4F1C9FC9BA7@iki.fi","threadId":"8229","inReplyTo":"20070520100155.GB3106@steel.home","subject":"Re: [PATCH 1/3] Added generic string handling code.","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-05-20T10:27:29Z","receivedAt":"2007-05-20T10:27:29Z","isPatch":true,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On 20.5.2007, at 13.01, Alex Riesen wrote:\n\n> Timo Sirainen, Sun, May 20, 2007 04:24:29 +0200:\n>> So here's my try on starting with something simple. Unlike almost all\n>> other string handling libraries, it doesn't allocate the memory\n>> dynamically.\n>\n> Sometimes you _need_ dinamic memory allocation.\n\nIt's easy to use the same str_*() functions to implement dynamic  \nmemory allocation. I think I could have done a bit different naming,  \nlike maybe:\n\nextern struct string *str_alloc(unsigned int len);\nextern void str_append(struct string *str, const char *cstr);\n\n#define static_string(name, size) ..\n#define sstr_append(str, cstr) str_append(&(str).string, cstr)\n\n\n>> This makes it really easy to convert existing code to use it. I'm\n>> including some example changes in the other patches. Besides making\n>> the code safer, it can also make it faster, especially those\n>> strcat() replacements.\n>\n> It is also bigger, heavier on stack and sometimes slower because of\n> more function calls involved.\n\nI would hardly call 8 extra bytes on stack heavier. Also if this was  \nused everywhere I wouldn't be surprised if it made the code faster,  \nbecause it would remove a lot of overflow checking code so more code  \nwill fit into L1 cache.\n\n\n\n"},{"id":"42982","messageId":"20070522134007.GK4489@pasky.or.cz","threadId":"8229","inReplyTo":"1179627869.32181.1284.camel@hurina","subject":"Re: [PATCH 1/3] Added generic string handling code.","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2007-05-22T13:40:07Z","receivedAt":"2007-05-22T13:40:07Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Sun, May 20, 2007 at 04:24:29AM CEST, Timo Sirainen wrote:\n> diff --git a/str.c b/str.c\n> new file mode 100644\n> index 0000000..d46e7f4\n> --- /dev/null\n> +++ b/str.c\n> @@ -0,0 +1,40 @@\n> +#include \"str.h\"\n> +\n> +void _str_append(struct string *str, const char *cstr)\n\n_ is reserved namespace.\n\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\nYou can copy len + 1 and avoid this assignment.\n\n> +}\n> +\n> +void _str_printfa(struct string *str, const char *fmt, ...)\n\nprintfA?\n\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 < 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-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nEver try. Ever fail. No matter. // Try again. Fail again. Fail better.\n\t\t-- Samuel Beckett\n"},{"id":"43027","messageId":"1179917386.32181.1643.camel@hurina","threadId":"8229","inReplyTo":"20070522134007.GK4489@pasky.or.cz","subject":"Re: [PATCH 1/3] Added generic string handling code.","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-05-23T10:49:46Z","receivedAt":"2007-05-23T10:49:46Z","isPatch":true,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On Tue, 2007-05-22 at 15:40 +0200, Petr Baudis wrote:\n> On Sun, May 20, 2007 at 04:24:29AM CEST, Timo Sirainen wrote:\n> > diff --git a/str.c b/str.c\n> > new file mode 100644\n> > index 0000000..d46e7f4\n> > --- /dev/null\n> > +++ b/str.c\n> > @@ -0,0 +1,40 @@\n> > +#include \"str.h\"\n> > +\n> > +void _str_append(struct string *str, const char *cstr)\n> \n> _ is reserved namespace.\n\nI remember __ is, but was _ too? A lot of programs are using that. :)\n\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> You can copy len + 1 and avoid this assignment.\n\nNot if the string overflowed.\n\n> > +}\n> > +\n> > +void _str_printfa(struct string *str, const char *fmt, ...)\n> \n> printfA?\n\n\"append\". I think I got it originally from glib:\n\n/* These aliases are included for compatibility. */\n#define g_string_sprintf g_string_printf\n#define g_string_sprintfa g_string_append_printf\n\nIf there's a chance that this string handling code would get used, I\ncould write another patch with a bit clearer names and support for\ndynamically growing strings too. Something like:\n\nSTATIC_STRING(name, 1234);\nsstr_append(name, \"hello\"); // or static_str_append()?\n\nstruct string *dyn = str_new(1024); // initial length\nstr_append(dyn, \"hello\");\n\n"},{"id":"43029","messageId":"20070523132429.GM4489@pasky.or.cz","threadId":"8229","inReplyTo":"1179917386.32181.1643.camel@hurina","subject":"Re: [PATCH 1/3] Added generic string handling code.","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2007-05-23T13:24:29Z","receivedAt":"2007-05-23T13:24:29Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Wed, May 23, 2007 at 12:49:46PM CEST, Timo Sirainen wrote:\n> On Tue, 2007-05-22 at 15:40 +0200, Petr Baudis wrote:\n> > On Sun, May 20, 2007 at 04:24:29AM CEST, Timo Sirainen wrote:\n> > > diff --git a/str.c b/str.c\n> > > new file mode 100644\n> > > index 0000000..d46e7f4\n> > > --- /dev/null\n> > > +++ b/str.c\n> > > @@ -0,0 +1,40 @@\n> > > +#include \"str.h\"\n> > > +\n> > > +void _str_append(struct string *str, const char *cstr)\n> > \n> > _ is reserved namespace.\n> \n> I remember __ is, but was _ too? A lot of programs are using that. :)\n\nC99 7.1.3 says that\n\n   -- All identifiers that begin with an underscore are always reserved\nfor use as identifiers with file scope in both the ordinary and tag name\nspaces.\n\nHmm, this _is_ file scope, right?\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nEver try. Ever fail. No matter. // Try again. Fail again. Fail better.\n\t\t-- Samuel Beckett\n"},{"id":"43030","messageId":"1179928610.32181.1707.camel@hurina","threadId":"8229","inReplyTo":"20070523132429.GM4489@pasky.or.cz","subject":"Re: [PATCH 1/3] Added generic string handling code.","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-05-23T13:56:50Z","receivedAt":"2007-05-23T13:56:50Z","isPatch":true,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On Wed, 2007-05-23 at 15:24 +0200, Petr Baudis wrote:\n> > > > +void _str_append(struct string *str, const char *cstr)\n> > > \n> > > _ is reserved namespace.\n> > \n> > I remember __ is, but was _ too? A lot of programs are using that. :)\n> \n> C99 7.1.3 says that\n> \n>    -- All identifiers that begin with an underscore are always reserved\n> for use as identifiers with file scope in both the ordinary and tag name\n> spaces.\n> \n> Hmm, this _is_ file scope, right?\n\nRight. I'll start changing my practices. Although grepping\nunder /usr/include shows that there are a lot of other software that\ndoesn't respect it either (X11, MySQL, OpenSSL at least).\n\n"}]}