{"thread":{"id":"10041","subject":"Use of strbuf.buf when strbuf.len == 0","startedAt":"2007-09-27T06:21:24Z","lastAt":"2007-09-29T07:48:13Z","messageCount":10,"participants":["Junio C Hamano","Pierre Habouzit","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"54138","messageId":"7vir5wy6fv.fsf@gitster.siamese.dyndns.org","threadId":"10041","inReplyTo":null,"subject":"Use of strbuf.buf when strbuf.len == 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-27T06:21:24Z","receivedAt":"2007-09-27T06:21:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I am starting to hate some parts of the strbuf API.\n\nHere is an example.  Can you spot what goes wrong?\n\nstatic int handle_file(const char *path,\n\t unsigned char *sha1, const char *output)\n{\n\tSHA_CTX ctx;\n\tchar buf[1024];\n\tint hunk = 0, hunk_no = 0;\n\tstruct strbuf one, two;\n\t...\n\tif (sha1)\n\t\tSHA1_Init(&ctx);\n\n\tstrbuf_init(&one, 0);\n\tstrbuf_init(&two,  0);\n\twhile (fgets(buf, sizeof(buf), f)) {\n\t\tif (!prefixcmp(buf, \"<<<<<<< \"))\n\t\t\thunk = 1;\n\t\telse if (!prefixcmp(buf, \"=======\"))\n\t\t\thunk = 2;\n\t\telse if (!prefixcmp(buf, \">>>>>>> \")) {\n\t\t\tint cmp = strbuf_cmp(&one, &two);\n\n\t\t\thunk_no++;\n\t\t\thunk = 0;\n\t\t\tif (cmp > 0) {\n\t\t\t\tstrbuf_swap(&one, &two);\n\t\t\t}\n\t\t\tif (out) {\n\t\t\t\tfputs(\"<<<<<<<\\n\", out);\n\t\t\t\tfwrite(one.buf, one.len, 1, out);\n\t\t\t\tfputs(\"=======\\n\", out);\n\t\t\t\tfwrite(two.buf, two.len, 1, out);\n\t\t\t\tfputs(\">>>>>>>\\n\", out);\n\t\t\t}\n\t\t\tif (sha1) {\n\t\t\t\tSHA1_Update(&ctx, one.buf, one.len + 1);\n\t\t\t\tSHA1_Update(&ctx, two.buf, two.len + 1);\n\t\t\t}\n\t\t\tstrbuf_reset(&one);\n\t\t\tstrbuf_reset(&two);\n\t\t} else if (hunk == 1)\n\t\t\tstrbuf_addstr(&one, buf);\n\t\telse if (hunk == 2)\n\t\t\tstrbuf_addstr(&two, buf);\n\t\telse if (out)\n\t\t\tfputs(buf, out);\n\t}\n\tstrbuf_release(&one);\n\tstrbuf_release(&two);\n\t...\n}\n\nWhen one or two are empty and the caller asked for checksumming,\nthe code still relies on one.buf being an allocated memory with\nan extra NUL termination and tries to feed the NULL pointer to\nSHA1_Update() with length of 1!\n\nAn obvious workaround is to say \"strbuf_init(&one, !!sha1)\" to\nforce the allocation.  However, because the second parameter to\nstrbuf_init() is defined to be merely a hint, we should not rely\non strbuf_init() with non-zero hint to have allocation from the\nbeginning.  It is assuming too much.\n\nA more defensive way would be for the user to do something like:\n\n\tSHA1_Update(&ctx, one.buf ? one.buf : \"\", one.len + 1);\n\tSHA1_Update(&ctx, two.buf ? two.buf : \"\", two.len + 1);\n\nwhich I am leaning towards, but this looks ugly.\n\nI was already bitten by another bug in strbuf_setlen() that\nshared the same roots; an empty strbuf can have two internal\nrepresentations (\"alloc == 0 && buf == NULL\" vs \"alloc != 0 &&\nbuf != NULL\") and they behave differently.\n\nFor example, this happens to be Ok but the validity of it is\nsubtle.  If a or b is empty, memcmp may get a NULL pointer but\nwe ask only 0 byte comparison.\n\n        int strbuf_cmp(struct strbuf *a, struct strbuf *b)\n        {\n                int cmp;\n                if (a->len < b->len) {\n                        cmp = memcmp(a->buf, b->buf, a->len);\n                        return cmp ? cmp : -1;\n                } else {\n                        cmp = memcmp(a->buf, b->buf, b->len);\n                        return cmp ? cmp : a->len != b->len;\n                }\n        }\n\nIt might be an easier and safer fix to define that strbuf_init()\nto always have allocation.  Use of a strbuf in the code _and_\nnot adding any contents to the buffer should be an exception and\navoiding malloc()/free() for that special case feels optimizing\nfor the wrong case.\n\nHowever, there are strbuf instances that are not initialized\n(i.e. in BSS or initialized by declaring with STRBUF_INIT), so\nwe still need to handle (.len == 0 && .alloc == 0) case\nspecially anyway.\n\nIt would be appreciated if somebody with a fresh pair of eyes\ncan go over the strbuf series one more time to make sure that we\ndo not try to blindly use buf.buf, assuming buf.buf[0] is NUL if\n(buf.len == 0).\n"},{"id":"54150","messageId":"20070927101300.GD10289@artemis.corp","threadId":"10041","inReplyTo":"7vir5wy6fv.fsf@gitster.siamese.dyndns.org","subject":"Re: Use of strbuf.buf when strbuf.len == 0","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T10:13:00Z","receivedAt":"2007-09-27T10:13:00Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Sep 27, 2007 at 06:21:24AM +0000, Junio C Hamano wrote:\n> It might be an easier and safer fix to define that strbuf_init()\n> to always have allocation.  Use of a strbuf in the code _and_\n> not adding any contents to the buffer should be an exception and\n> avoiding malloc()/free() for that special case feels optimizing\n> for the wrong case.\n> \n> However, there are strbuf instances that are not initialized\n> (i.e. in BSS or initialized by declaring with STRBUF_INIT), so\n> we still need to handle (.len == 0 && .alloc == 0) case\n> specially anyway.\n\n  I can see a way, that would need special proof-reading of the strbuf\nmodule, but should not harm its users, that would be to change\nSTRBUF_INIT to work this way:\n\n  { .buf = \"\", .len = 0, .alloc = 0 }\n\n  It needs to make strbuf_grow and strbuf_release check for ->alloc\nbefore doing anything stupid.\n\n  Though we may have some bits of code that rely on .buf being NULL if\nnothing happened. I tried to track them down, but some may remain.\n\n  If you agree with this change, that would solve most of the issues\nwith almost no cost, then I'll propose a new patch with this change.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"54152","messageId":"20070927110456.50993930B@madism.org","threadId":"10041","inReplyTo":"20070927101300.GD10289@artemis.corp","subject":"[PATCH 1/2] double free in builtin-update-index.c","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T10:51:18Z","receivedAt":"2007-09-27T10:51:18Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"path_name is either ptr that should not be freed, or a pointer to a strbuf\nbuffer that is deallocated when exiting the loop. Don't do that !\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n builtin-update-index.c |    2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex c76879e..e1a938d 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -377,8 +377,6 @@ static void read_index_info(int line_termination)\n \t\t\t\tdie(\"git-update-index: unable to update %s\",\n \t\t\t\t    path_name);\n \t\t}\n-\t\tif (path_name != ptr)\n-\t\t\tfree(path_name);\n \t\tcontinue;\n \n \tbad_line:\n-- \n1.5.3.2.1100.g015a-dirty\n"},{"id":"54153","messageId":"20070927110940.F2C9D930B@madism.org","threadId":"10041","inReplyTo":"20070927101300.GD10289@artemis.corp","subject":"[PATCH 2/2] strbuf change: be sure ->buf is never ever NULL.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T10:58:23Z","receivedAt":"2007-09-27T10:58:23Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"For that purpose, the ->buf is always initialized with a char * buf living\nin the strbuf module. It is made a char * so that we can sloppily accept\nthings that perform: sb->buf[0] = '\\0', and because you can't pass \"\" as an\ninitializer for ->buf without making gcc unhappy for very good reasons.\n\nstrbuf_init/_detach/_grow have been fixed to trust ->alloc and not ->buf\nanymore.\n\nas a consequence strbuf_detach is _mandatory_ to detach a buffer, copying\n->buf isn't an option anymore, if ->buf is going to escape from the scope,\nand eventually be free'd.\n\nAPI changes:\n  * strbuf_setlen now always works, so just make strbuf_reset a convenience\n    macro.\n  * strbuf_detatch takes a size_t* optional argument (meaning it can be\n    NULL) to copy the buffer's len, as it was needed for this refactor to\n    make the code more readable, and working like the callers.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n\nI've written this patch using the following method:\n\n(1) I've renamed the ->buf member into sb_buf (I grepped before it wasn't\n    a string used in the project) and made the changes I describe in\n    strbuf.[hc]\n\n(2) I've built the project, and renamed every ->buf into sb_buf, and\n    applied the needed semantics changes. It's doing that that I found the\n    issue I fix in the patch 1/2.\n\n(3) I've then run the testsuite, it passes.\n\n(4) I've sed -i -e 's/\\<sb_buf\\>/buf/g' *.h *.c\n\n\nNobody was directly using the fact that a strbuf that wasn't touched had\nits pointer NULL, though people detach'ing them do, and strbuf_detach\ncomplies with that. That's why I think, despite the somehow tasteless\n\"strbuf_slopbuf\" it's the best way to go.\n\n builtin-apply.c       |   16 +++++++---------\n builtin-archive.c     |    5 ++---\n builtin-fetch--tool.c |    2 +-\n commit.c              |    2 +-\n convert.c             |    4 ++--\n diff.c                |   14 ++++++--------\n entry.c               |    3 +--\n fast-import.c         |    2 +-\n imap-send.c           |    2 +-\n quote.c               |    2 +-\n sha1_file.c           |    3 +--\n strbuf.c              |   27 ++++++++++++++++-----------\n strbuf.h              |   10 +++++-----\n 13 files changed, 45 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 450f0a8..833b142 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -178,14 +178,13 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c\n #define CHUNKSIZE (8192)\n #define SLOP (16)\n \n-static void *read_patch_file(int fd, unsigned long *sizep)\n+static void *read_patch_file(int fd, size_t *sizep)\n {\n \tstruct strbuf buf;\n \n \tstrbuf_init(&buf, 0);\n \tif (strbuf_read(&buf, fd, 0) < 0)\n \t\tdie(\"git-apply: read returned %s\", strerror(errno));\n-\t*sizep = buf.len;\n \n \t/*\n \t * Make sure that we have some slop in the buffer\n@@ -194,7 +193,7 @@ static void *read_patch_file(int fd, unsigned long *sizep)\n \t */\n \tstrbuf_grow(&buf, SLOP);\n \tmemset(buf.buf + buf.len, 0, SLOP);\n-\treturn strbuf_detach(&buf);\n+\treturn strbuf_detach(&buf, sizep);\n }\n \n static unsigned long linelen(const char *buffer, unsigned long size)\n@@ -253,7 +252,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)\n \t\t\t\t */\n \t\t\t\tstrbuf_remove(&name, 0, cp - name.buf);\n \t\t\t\tfree(def);\n-\t\t\t\treturn name.buf;\n+\t\t\t\treturn strbuf_detach(&name, NULL);\n \t\t\t}\n \t\t}\n \t\tstrbuf_release(&name);\n@@ -607,7 +606,7 @@ static char *git_header_name(char *line, int llen)\n \t\t\tif (strcmp(cp + 1, first.buf))\n \t\t\t\tgoto free_and_fail1;\n \t\t\tstrbuf_release(&sp);\n-\t\t\treturn first.buf;\n+\t\t\treturn strbuf_detach(&first, NULL);\n \t\t}\n \n \t\t/* unquoted second */\n@@ -618,7 +617,7 @@ static char *git_header_name(char *line, int llen)\n \t\tif (line + llen - cp != first.len + 1 ||\n \t\t    memcmp(first.buf, cp, first.len))\n \t\t\tgoto free_and_fail1;\n-\t\treturn first.buf;\n+\t\treturn strbuf_detach(&first, NULL);\n \n \tfree_and_fail1:\n \t\tstrbuf_release(&first);\n@@ -655,7 +654,7 @@ static char *git_header_name(char *line, int llen)\n \t\t\t    isspace(name[len])) {\n \t\t\t\t/* Good */\n \t\t\t\tstrbuf_remove(&sp, 0, np - sp.buf);\n-\t\t\t\treturn sp.buf;\n+\t\t\t\treturn strbuf_detach(&sp, NULL);\n \t\t\t}\n \n \t\tfree_and_fail2:\n@@ -1968,8 +1967,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \n \tif (apply_fragments(&buf, patch) < 0)\n \t\treturn -1; /* note with --reject this succeeds. */\n-\tpatch->result = buf.buf;\n-\tpatch->resultsize = buf.len;\n+\tpatch->result = strbuf_detach(&buf, &patch->resultsize);\n \n \tif (0 < patch->is_delete && patch->resultsize)\n \t\treturn error(\"removal patch leaves file contents\");\ndiff --git a/builtin-archive.c b/builtin-archive.c\nindex 843a9e3..04385de 100644\n--- a/builtin-archive.c\n+++ b/builtin-archive.c\n@@ -89,7 +89,7 @@ static void format_subst(const struct commit *commit,\n \tstruct strbuf fmt;\n \n \tif (src == buf->buf)\n-\t\tto_free = strbuf_detach(buf);\n+\t\tto_free = strbuf_detach(buf, NULL);\n \tstrbuf_init(&fmt, 0);\n \tfor (;;) {\n \t\tconst char *b, *c;\n@@ -153,8 +153,7 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,\n \t\tstrbuf_attach(&buf, buffer, *sizep, *sizep + 1);\n \t\tconvert_to_working_tree(path, buf.buf, buf.len, &buf);\n \t\tconvert_to_archive(path, buf.buf, buf.len, &buf, commit);\n-\t\t*sizep = buf.len;\n-\t\tbuffer = buf.buf;\n+\t\tbuffer = strbuf_detach(&buf, sizep);\n \t}\n \n \treturn buffer;\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex 349b59c..1e43d79 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -10,7 +10,7 @@ static char *get_stdin(void)\n \tif (strbuf_read(&buf, 0, 1024) < 0) {\n \t\tdie(\"error reading standard input: %s\", strerror(errno));\n \t}\n-\treturn strbuf_detach(&buf);\n+\treturn strbuf_detach(&buf, NULL);\n }\n \n static void show_new(enum object_type type, unsigned char *sha1_new)\ndiff --git a/commit.c b/commit.c\nindex 1e391e6..20fb220 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -663,7 +663,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \t\t\t\t\t  len - strlen(\"encoding \\n\"),\n \t\t\t\t\t  encoding, strlen(encoding));\n \t}\n-\treturn tmp.buf;\n+\treturn strbuf_detach(&tmp, NULL);\n }\n \n static char *logmsg_reencode(const struct commit *commit,\ndiff --git a/convert.c b/convert.c\nindex 79c9df2..0d5e909 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -168,7 +168,7 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n \n \t/* are we \"faking\" in place editing ? */\n \tif (src == buf->buf)\n-\t\tto_free = strbuf_detach(buf);\n+\t\tto_free = strbuf_detach(buf, NULL);\n \n \tstrbuf_grow(buf, len + stats.lf - stats.crlf);\n \tfor (;;) {\n@@ -464,7 +464,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,\n \n \t/* are we \"faking\" in place editing ? */\n \tif (src == buf->buf)\n-\t\tto_free = strbuf_detach(buf);\n+\t\tto_free = strbuf_detach(buf, NULL);\n \thash_sha1_file(src, len, \"blob\", sha1);\n \n \tstrbuf_grow(buf, len + cnt * 43);\ndiff --git a/diff.c b/diff.c\nindex 4a7f1e1..0bd7e24 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -197,7 +197,7 @@ static char *quote_two(const char *one, const char *two)\n \t\tstrbuf_addstr(&res, one);\n \t\tstrbuf_addstr(&res, two);\n \t}\n-\treturn res.buf;\n+\treturn strbuf_detach(&res, NULL);\n }\n \n static const char *external_diff(void)\n@@ -662,7 +662,7 @@ static char *pprint_rename(const char *a, const char *b)\n \t\tquote_c_style(a, &name, NULL, 0);\n \t\tstrbuf_addstr(&name, \" => \");\n \t\tquote_c_style(b, &name, NULL, 0);\n-\t\treturn name.buf;\n+\t\treturn strbuf_detach(&name, NULL);\n \t}\n \n \t/* Find common prefix */\n@@ -710,7 +710,7 @@ static char *pprint_rename(const char *a, const char *b)\n \t\tstrbuf_addch(&name, '}');\n \t\tstrbuf_add(&name, a + len_a - sfx_length, sfx_length);\n \t}\n-\treturn name.buf;\n+\treturn strbuf_detach(&name, NULL);\n }\n \n struct diffstat_t {\n@@ -827,7 +827,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)\n \t\t\tstrbuf_init(&buf, 0);\n \t\t\tif (quote_c_style(file->name, &buf, NULL, 0)) {\n \t\t\t\tfree(file->name);\n-\t\t\t\tfile->name = buf.buf;\n+\t\t\t\tfile->name = strbuf_detach(&buf, NULL);\n \t\t\t} else {\n \t\t\t\tstrbuf_release(&buf);\n \t\t\t}\n@@ -1519,8 +1519,7 @@ static int populate_from_stdin(struct diff_filespec *s)\n \t\t\t\t     strerror(errno));\n \n \ts->should_munmap = 0;\n-\ts->size = buf.len;\n-\ts->data = strbuf_detach(&buf);\n+\ts->data = strbuf_detach(&buf, &s->size);\n \ts->should_free = 1;\n \treturn 0;\n }\n@@ -1612,8 +1611,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\tif (convert_to_git(s->path, s->data, s->size, &buf)) {\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\n-\t\t\ts->data = buf.buf;\n-\t\t\ts->size = buf.len;\n+\t\t\ts->data = strbuf_detach(&buf, &s->size);\n \t\t\ts->should_free = 1;\n \t\t}\n \t}\ndiff --git a/entry.c b/entry.c\nindex 4a8c73b..98f5f6d 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -120,8 +120,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\tstrbuf_init(&buf, 0);\n \t\tif (convert_to_working_tree(ce->name, new, size, &buf)) {\n \t\t\tfree(new);\n-\t\t\tnew = buf.buf;\n-\t\t\tsize = buf.len;\n+\t\t\tnew = strbuf_detach(&buf, &size);\n \t\t}\n \n \t\tif (to_tempfile) {\ndiff --git a/fast-import.c b/fast-import.c\nindex a870a44..e9c80be 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1562,7 +1562,7 @@ static int read_next_command(void)\n \t\t} else {\n \t\t\tstruct recent_command *rc;\n \n-\t\t\tstrbuf_detach(&command_buf);\n+\t\t\tstrbuf_detach(&command_buf, NULL);\n \t\t\tstdin_eof = strbuf_getline(&command_buf, stdin, '\\n');\n \t\t\tif (stdin_eof)\n \t\t\t\treturn EOF;\ndiff --git a/imap-send.c b/imap-send.c\nindex e95cdde..a429a76 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1180,7 +1180,7 @@ read_message( FILE *f, msg_data_t *msg )\n \t} while (!feof(f));\n \n \tmsg->len  = buf.len;\n-\tmsg->data = strbuf_detach(&buf);\n+\tmsg->data = strbuf_detach(&buf, NULL);\n \treturn msg->len;\n }\n \ndiff --git a/quote.c b/quote.c\nindex 800fd88..482be05 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -22,7 +22,7 @@ void sq_quote_buf(struct strbuf *dst, const char *src)\n \tchar *to_free = NULL;\n \n \tif (dst->buf == src)\n-\t\tto_free = strbuf_detach(dst);\n+\t\tto_free = strbuf_detach(dst, NULL);\n \n \tstrbuf_addch(dst, '\\'');\n \twhile (*src) {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f1377fb..83a06a7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2340,8 +2340,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t\tstrbuf_init(&nbuf, 0);\n \t\tif (convert_to_git(path, buf, size, &nbuf)) {\n \t\t\tmunmap(buf, size);\n-\t\t\tsize = nbuf.len;\n-\t\t\tbuf = nbuf.buf;\n+\t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\n \t\t}\n \t}\ndiff --git a/strbuf.c b/strbuf.c\nindex d5e92ee..450110d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,27 +1,30 @@\n #include \"cache.h\"\n \n+/* used as the default ->buf value, so that people can always assume buf is\n+   non NULL and ->buf[0] is '\\0' */\n+char strbuf_slopbuf[32];\n+\n void strbuf_init(struct strbuf *sb, size_t hint)\n {\n-\tmemset(sb, 0, sizeof(*sb));\n+\tsb->alloc = sb->len = 0;\n+\tsb->buf = strbuf_slopbuf;\n \tif (hint)\n \t\tstrbuf_grow(sb, hint);\n }\n \n void strbuf_release(struct strbuf *sb)\n {\n-\tfree(sb->buf);\n-\tmemset(sb, 0, sizeof(*sb));\n-}\n-\n-void strbuf_reset(struct strbuf *sb)\n-{\n-\tif (sb->len)\n-\t\tstrbuf_setlen(sb, 0);\n+\tif (sb->alloc) {\n+\t\tfree(sb->buf);\n+\t\tstrbuf_init(sb, 0);\n+\t}\n }\n \n-char *strbuf_detach(struct strbuf *sb)\n+char *strbuf_detach(struct strbuf *sb, size_t *sz)\n {\n-\tchar *res = sb->buf;\n+\tchar *res = sb->alloc ? sb->buf : NULL;\n+\tif (sz)\n+\t\t*sz = sb->len;\n \tstrbuf_init(sb, 0);\n \treturn res;\n }\n@@ -40,6 +43,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n \tif (sb->len + extra + 1 <= sb->len)\n \t\tdie(\"you want to use way too much memory\");\n+\tif (!sb->alloc)\n+\t\tsb->buf = NULL;\n \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n }\n \ndiff --git a/strbuf.h b/strbuf.h\nindex fd68389..a92222b 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -10,8 +10,7 @@\n  * 1. the ->buf member is always malloc-ed, hence strbuf's can be used to\n  *    build complex strings/buffers whose final size isn't easily known.\n  *\n- *    It is legal to copy the ->buf pointer away. Though if you want to reuse\n- *    the strbuf after that, setting ->buf to NULL isn't legal.\n+ *    It is NOT legal to copy the ->buf pointer away.\n  *    `strbuf_detach' is the operation that detachs a buffer from its shell\n  *    while keeping the shell valid wrt its invariants.\n  *\n@@ -41,19 +40,19 @@\n \n #include <assert.h>\n \n+extern char strbuf_slopbuf[];\n struct strbuf {\n \tsize_t alloc;\n \tsize_t len;\n \tchar *buf;\n };\n \n-#define STRBUF_INIT  { 0, 0, NULL }\n+#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }\n \n /*----- strbuf life cycle -----*/\n extern void strbuf_init(struct strbuf *, size_t);\n extern void strbuf_release(struct strbuf *);\n-extern void strbuf_reset(struct strbuf *);\n-extern char *strbuf_detach(struct strbuf *);\n+extern char *strbuf_detach(struct strbuf *, size_t *);\n extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n static inline void strbuf_swap(struct strbuf *a, struct strbuf *b) {\n \tstruct strbuf tmp = *a;\n@@ -75,6 +74,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {\n \tsb->len = len;\n \tsb->buf[len] = '\\0';\n }\n+#define strbuf_reset(sb)  strbuf_setlen(sb, 0)\n \n /*----- content related -----*/\n extern void strbuf_rtrim(struct strbuf *);\n-- \n1.5.3.2.1100.g015a-dirty\n"},{"id":"54154","messageId":"20070927112204.GE10289@artemis.corp","threadId":"10041","inReplyTo":"7vir5wy6fv.fsf@gitster.siamese.dyndns.org","subject":"Re: Use of strbuf.buf when strbuf.len == 0","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T11:22:04Z","receivedAt":"2007-09-27T11:22:04Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Sep 27, 2007 at 06:21:24AM +0000, Junio C Hamano wrote:\n> It would be appreciated if somebody with a fresh pair of eyes\n> can go over the strbuf series one more time to make sure that we\n> do not try to blindly use buf.buf, assuming buf.buf[0] is NUL if\n> (buf.len == 0).\n\n  Like said in the 2/2 patch, I think it's better if people could be\nable to always assume that and be done with it, else you have to know\nthis internal duality of the empty strbuf and it sucks.\n\n  Instead, what is important, is that people that initialized a strbuf,\nthen want to go back in the char* world gets a NULL if nothing was\nallocated. It is a semantics that is used in a few places (it's arguable\nthat it's a right thing to assume though). For those, making\nstrbuf_detach use mandatory, and dealing with the special ->alloc == 0\ncase is the easiest way.\n\n  And as I don't trust my eyes to be fresh, I've used the aid of the\ncompiler to bust any place where we were using .buf members directly,\npossibly doing something stupid.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"54155","messageId":"20070927113347.5C4AE8D5A@madism.org","threadId":"10041","inReplyTo":"20070927112204.GE10289@artemis.corp","subject":"[PATCH 1/1] Make read_patch_file work on a strbuf.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T11:33:19Z","receivedAt":"2007-09-27T11:33:19Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"So that we don't need to use strbuf_detach.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n builtin-apply.c |   18 ++++++++----------\n 1 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 833b142..b0c6e60 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -178,7 +178,7 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c\n #define CHUNKSIZE (8192)\n #define SLOP (16)\n \n-static void *read_patch_file(int fd, size_t *sizep)\n+static void read_patch_file(struct strbuf *sb, int fd)\n {\n \tstruct strbuf buf;\n \n@@ -193,7 +193,6 @@ static void *read_patch_file(int fd, size_t *sizep)\n \t */\n \tstrbuf_grow(&buf, SLOP);\n \tmemset(buf.buf + buf.len, 0, SLOP);\n-\treturn strbuf_detach(&buf, sizep);\n }\n \n static unsigned long linelen(const char *buffer, unsigned long size)\n@@ -2648,22 +2647,22 @@ static void prefix_patches(struct patch *p)\n \n static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n {\n-\tunsigned long offset, size;\n-\tchar *buffer = read_patch_file(fd, &size);\n+\tsize_t offset;\n+\tstruct strbuf buf;\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \n+\tstrbuf_init(&buf, 0);\n \tpatch_input_file = filename;\n-\tif (!buffer)\n-\t\treturn -1;\n+\tread_patch_file(&buf, fd);\n \toffset = 0;\n-\twhile (size > 0) {\n+\twhile (offset < buf.len) {\n \t\tstruct patch *patch;\n \t\tint nr;\n \n \t\tpatch = xcalloc(1, sizeof(*patch));\n \t\tpatch->inaccurate_eof = inaccurate_eof;\n-\t\tnr = parse_chunk(buffer + offset, size, patch);\n+\t\tnr = parse_chunk(buf.buf + offset, buf.len, patch);\n \t\tif (nr < 0)\n \t\t\tbreak;\n \t\tif (apply_in_reverse)\n@@ -2681,7 +2680,6 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \t\t\tskipped_patch++;\n \t\t}\n \t\toffset += nr;\n-\t\tsize -= nr;\n \t}\n \n \tif (whitespace_error && (new_whitespace == error_on_whitespace))\n@@ -2716,7 +2714,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \tif (summary)\n \t\tsummary_patch_list(list);\n \n-\tfree(buffer);\n+\tstrbuf_release(&buf);\n \treturn 0;\n }\n \n-- \n1.5.3.2.1101.g89e60-dirty\n"},{"id":"54157","messageId":"20070927114016.08CCB8C4A@madism.org","threadId":"10041","inReplyTo":"20070927112204.GE10289@artemis.corp","subject":"[PROPER PATCH 1/1] Make read_patch_file work on a strbuf.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T11:33:19Z","receivedAt":"2007-09-27T11:33:19Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"So that we don't need to use strbuf_detach.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n\nSorry, in the hurry I sent a wrong patch before, this one works :)\n\n builtin-apply.c |   27 +++++++++++----------------\n 1 files changed, 11 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 833b142..1f0a672 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -178,12 +178,9 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c\n #define CHUNKSIZE (8192)\n #define SLOP (16)\n \n-static void *read_patch_file(int fd, size_t *sizep)\n+static void read_patch_file(struct strbuf *sb, int fd)\n {\n-\tstruct strbuf buf;\n-\n-\tstrbuf_init(&buf, 0);\n-\tif (strbuf_read(&buf, fd, 0) < 0)\n+\tif (strbuf_read(sb, fd, 0) < 0)\n \t\tdie(\"git-apply: read returned %s\", strerror(errno));\n \n \t/*\n@@ -191,9 +188,8 @@ static void *read_patch_file(int fd, size_t *sizep)\n \t * so that we can do speculative \"memcmp\" etc, and\n \t * see to it that it is NUL-filled.\n \t */\n-\tstrbuf_grow(&buf, SLOP);\n-\tmemset(buf.buf + buf.len, 0, SLOP);\n-\treturn strbuf_detach(&buf, sizep);\n+\tstrbuf_grow(sb, SLOP);\n+\tmemset(sb->buf + sb->len, 0, SLOP);\n }\n \n static unsigned long linelen(const char *buffer, unsigned long size)\n@@ -2648,22 +2644,22 @@ static void prefix_patches(struct patch *p)\n \n static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n {\n-\tunsigned long offset, size;\n-\tchar *buffer = read_patch_file(fd, &size);\n+\tsize_t offset;\n+\tstruct strbuf buf;\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \n+\tstrbuf_init(&buf, 0);\n \tpatch_input_file = filename;\n-\tif (!buffer)\n-\t\treturn -1;\n+\tread_patch_file(&buf, fd);\n \toffset = 0;\n-\twhile (size > 0) {\n+\twhile (offset < buf.len) {\n \t\tstruct patch *patch;\n \t\tint nr;\n \n \t\tpatch = xcalloc(1, sizeof(*patch));\n \t\tpatch->inaccurate_eof = inaccurate_eof;\n-\t\tnr = parse_chunk(buffer + offset, size, patch);\n+\t\tnr = parse_chunk(buf.buf + offset, buf.len, patch);\n \t\tif (nr < 0)\n \t\t\tbreak;\n \t\tif (apply_in_reverse)\n@@ -2681,7 +2677,6 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \t\t\tskipped_patch++;\n \t\t}\n \t\toffset += nr;\n-\t\tsize -= nr;\n \t}\n \n \tif (whitespace_error && (new_whitespace == error_on_whitespace))\n@@ -2716,7 +2711,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \tif (summary)\n \t\tsummary_patch_list(list);\n \n-\tfree(buffer);\n+\tstrbuf_release(&buf);\n \treturn 0;\n }\n \n-- \n1.5.3.2.1102.gefa87-dirty\n"},{"id":"54156","messageId":"20070927113718.GF10289@artemis.corp","threadId":"10041","inReplyTo":"20070927112204.GE10289@artemis.corp","subject":"Re: Use of strbuf.buf when strbuf.len == 0","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-27T11:37:18Z","receivedAt":"2007-09-27T11:37:18Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Sep 27, 2007 at 11:22:04AM +0000, Pierre Habouzit wrote:\n> (it's arguable > that it's a right thing to assume though)\n\n  I find this ugly, so I've even checked if we did assume that, which is\neasy now that such places all use strbuf_detach. One place seemed to,\nbut wasn't, so I patched it so that it's not the case anymore.\n\n  All other places that use strbuf_detach don't use the fact that it can\nreturn NULL to detect error cases, so we are safe.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"54307","messageId":"alpine.LFD.0.999.0709281746500.3579@woody.linux-foundation.org","threadId":"10041","inReplyTo":"20070927101300.GD10289@artemis.corp","subject":"Re: Use of strbuf.buf when strbuf.len == 0","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-29T00:51:36Z","receivedAt":"2007-09-29T00:51:36Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 27 Sep 2007, Pierre Habouzit wrote:\n> \n>   I can see a way, that would need special proof-reading of the strbuf\n> module, but should not harm its users, that would be to change\n> STRBUF_INIT to work this way:\n> \n>   { .buf = \"\", .len = 0, .alloc = 0 }\n\nI'd like to pipe up a bit here..\n\nI think the above is a good fix for the current problem of wanting to \nalways be able to use \"sb->buf\", but I thinkit actually has the potential \nto fix another issue entirely.\n\nNamely strbuf's that are initialized from various static strings and/or \nstrings not directly allocated with malloc().\n\nThat's not necessarily something really unusual. Wanting to initialize a \nstring with a fixed constant value is a common problem.\n\nAnd wouldn't it be nice if you could actually do that, with\n\n\t{ .buf = \"static initializer\", .len = 18, .alloc = 0 }\n\nand have all the strbuf routines that modify the initializer (including \nmaking it shorter!) notice that the allocation is too short, and create a \nnew allocation?\n\nHmm?\n\n\t\t\tLinus\n"},{"id":"54316","messageId":"20070929074813.GA3366@artemis.corp","threadId":"10041","inReplyTo":"alpine.LFD.0.999.0709281746500.3579@woody.linux-foundation.org","subject":"Re: Use of strbuf.buf when strbuf.len == 0","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-29T07:48:13Z","receivedAt":"2007-09-29T07:48:13Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Sep 29, 2007 at 12:51:36AM +0000, Linus Torvalds wrote:\n> \n> \n> On Thu, 27 Sep 2007, Pierre Habouzit wrote:\n> > \n> >   I can see a way, that would need special proof-reading of the strbuf\n> > module, but should not harm its users, that would be to change\n> > STRBUF_INIT to work this way:\n> > \n> >   { .buf = \"\", .len = 0, .alloc = 0 }\n> \n> I'd like to pipe up a bit here..\n> \n> I think the above is a good fix for the current problem of wanting to \n> always be able to use \"sb->buf\", but I thinkit actually has the potential \n> to fix another issue entirely.\n> \n> Namely strbuf's that are initialized from various static strings and/or \n> strings not directly allocated with malloc().\n> \n> That's not necessarily something really unusual. Wanting to initialize a \n> string with a fixed constant value is a common problem.\n> \n> And wouldn't it be nice if you could actually do that, with\n> \n> \t{ .buf = \"static initializer\", .len = 18, .alloc = 0 }\n> \n> and have all the strbuf routines that modify the initializer (including \n> making it shorter!) notice that the allocation is too short, and create a \n> new allocation?\n\n  We could probably do that. The places to change to make this work are\nseldom:\n  * strbuf_grow to emulate a realloc (and copy the buffer into the new\n    malloc()ed one),\n  * strbuf_release assumes that ->alloc == 0 means _init isn't\n    necessary, it would be now,\n  * strbuf_setlen should not have the assert anymore (though I'm not\n    sure this assert still makes sense with the new initializer).\nand that's it.\n\n  But we cannot initialize a strbuf with an immediate string because all\nthe strbuf APIs suppose that the strbuf buffer are writeable (and IMHO\nit's pointless to use a strbuf for reading purposes). Other point, I've\nmade many readings of the code searching for specific patterns of code,\nto replace with strbuf's, and I've never seen places (I do not say those\ndon't exists though) that would directly benefit from that.\n\n> Hmm?\n\n  So I'd say I'll keep the idea in mind, because it's tasteful and could\nhelp, though I'd prefer Junio to review that patch, and then later add\nthis new semantics if the need arises.\n\n  The sole thing that may be worth investigating would look like:\n\n      char internal_buf[PATH_MAX]; /* should be damn long enough */\n      struct strbuf buf;\n\n      strbuf_init_static(&buf, internal_buf, sizeof(internal_buf);\n\n      /* hack with the strbuf API */\n\n      strbuf_release(&buf); /* do release memory, in case we went over\n                               PATH_MAX octets */\n\nbecause it could save some allocations _and_ be safe at the same time.\nBut I don't really like it, when allocation is critical in git, it's\nrarely in functions where there is an obvious size limit for the\nproblem, and avoiding allocations can be done using static strbufs\n(fast-import.c does that in many places).\n\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"}]}