{"thread":{"id":"14479","subject":"[PATCH] Fix buffer overflow in git-grep","startedAt":"2008-07-16T10:15:45Z","lastAt":"2008-07-16T15:39:55Z","messageCount":10,"participants":["Dmitry Potapov","Johannes Schindelin","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"83491","messageId":"1216203345-18233-1-git-send-email-dpotapov@gmail.com","threadId":"14479","inReplyTo":null,"subject":"[PATCH] Fix buffer overflow in git-grep","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T10:15:45Z","receivedAt":"2008-07-16T10:15:45Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If PATH_MAX on your system is smaller than any path stored in the git\nrepository, that can cause memory corruption inside of the grep_tree\nfunction used by git-grep.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n builtin-grep.c |    7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex ef29910..530a53d 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -441,14 +441,17 @@ static int grep_tree(struct grep_opt *opt, const char **paths,\n \tlen = strlen(path_buf);\n \n \twhile (tree_entry(tree, &entry)) {\n-\t\tstrcpy(path_buf + len, entry.path);\n+\t\tint te_len = tree_entry_len(entry.path, entry.sha1);\n+\t\tif (len + te_len >= PATH_MAX + tn_len)\n+\t\t\tdie (\"path too long: %s\", path_buf+tn_len);\n+\t\tmemcpy(path_buf + len, entry.path, te_len);\n \n \t\tif (S_ISDIR(entry.mode))\n \t\t\t/* Match \"abc/\" against pathspec to\n \t\t\t * decide if we want to descend into \"abc\"\n \t\t\t * directory.\n \t\t\t */\n-\t\t\tstrcpy(path_buf + len + tree_entry_len(entry.path, entry.sha1), \"/\");\n+\t\t\tstrcpy(path_buf + len + te_len, \"/\");\n \n \t\tif (!pathspec_matches(paths, down))\n \t\t\t;\n-- \n1.5.6.3.1.gb5587a\n"},{"id":"83495","messageId":"alpine.DEB.1.00.0807161232110.8503@eeepc-johanness","threadId":"14479","inReplyTo":"1216203345-18233-1-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] Fix buffer overflow in git-grep","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-16T10:35:06Z","receivedAt":"2008-07-16T10:35:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 16 Jul 2008, Dmitry Potapov wrote:\n\n> If PATH_MAX on your system is smaller than any path stored in the git \n> repository, that can cause memory corruption inside of the grep_tree \n> function used by git-grep.\n\nLet me guess: Windows? *giggles*\n\n> diff --git a/builtin-grep.c b/builtin-grep.c\n> index ef29910..530a53d 100644\n> --- a/builtin-grep.c\n> +++ b/builtin-grep.c\n> @@ -441,14 +441,17 @@ static int grep_tree(struct grep_opt *opt, const char **paths,\n>  \tlen = strlen(path_buf);\n>  \n>  \twhile (tree_entry(tree, &entry)) {\n> -\t\tstrcpy(path_buf + len, entry.path);\n> +\t\tint te_len = tree_entry_len(entry.path, entry.sha1);\n> +\t\tif (len + te_len >= PATH_MAX + tn_len)\n> +\t\t\tdie (\"path too long: %s\", path_buf+tn_len);\n> +\t\tmemcpy(path_buf + len, entry.path, te_len);\n\nThat is brutal.  Does grep_tree() not work on tree objects in memory?  In \nthat case, you prevent the user from grepping, only because she is on a \nsuboptimal platform, _even if_ even that platform could cope with it.\n\nIt's not like the path is ever used to access a file, right?\n\nMaybe you should convert the path_buf to a strbuf instead.\n\nCiao,\nDscho\n"},{"id":"83507","messageId":"20080716115420.GD2925@dpotapov.dyndns.org","threadId":"14479","inReplyTo":"alpine.DEB.1.00.0807161232110.8503@eeepc-johanness","subject":"Re: [PATCH] Fix buffer overflow in git-grep","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T11:54:20Z","receivedAt":"2008-07-16T11:54:20Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Wed, Jul 16, 2008 at 12:35:06PM +0200, Johannes Schindelin wrote:\n> >  \n> >  \twhile (tree_entry(tree, &entry)) {\n> > -\t\tstrcpy(path_buf + len, entry.path);\n> > +\t\tint te_len = tree_entry_len(entry.path, entry.sha1);\n> > +\t\tif (len + te_len >= PATH_MAX + tn_len)\n> > +\t\t\tdie (\"path too long: %s\", path_buf+tn_len);\n> > +\t\tmemcpy(path_buf + len, entry.path, te_len);\n> \n> That is brutal.  Does grep_tree() not work on tree objects in memory?  In \n> that case, you prevent the user from grepping, only because she is on a \n> suboptimal platform, _even if_ even that platform could cope with it.\n\nSure, but other git commands do not work much better in this case.\nIn fact, what you called as \"brutal\" may be considered as very\npolite comparing to what other git commands did.\n\nFor instance, git show will show you nothing at all and exit with 0.\nThe same problem with git whatchanged. The whole history mysteriously\ndisappeared at that commit, and git whatchanged exited with 0 without\nany error or warning... Though git log will show you all history, but\nif you run it with -p then it will also exit with zero at this commit\nsilently like previously history do not exist at all. So, I didn't see\nany reason to make git grep to work in the situation where practically\nany other git command does not. I guess, they should be corrected too,\nbut I did not have time to look at them yet.\n\n> \n> It's not like the path is ever used to access a file, right?\n> \n> Maybe you should convert the path_buf to a strbuf instead.\n\nIt is probably a good suggestion, but I just wanted to provided a quick\nfix to what may be considered as security issue. Of course, you usually\ndo not grep on untrusted repos, but if you did and something nasty\nhappened to you. I don't think it will help Git's reputation as being\nsecure and reliable...\n\nNow the question is whether we really want to fix all Git commands that\ndo not touch the work tree to work with filenames longer than PATH_MAX?\n\nDmitry\n"},{"id":"83519","messageId":"37fcd2780807160733o156e49c6m51314ff18764ccd5@mail.gmail.com","threadId":"14479","inReplyTo":"20080716115420.GD2925@dpotapov.dyndns.org","subject":"Re: [PATCH] Fix buffer overflow in git-grep","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T14:33:37Z","receivedAt":"2008-07-16T14:33:37Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Wed, Jul 16, 2008 at 3:54 PM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n>\n> For instance, git show will show you nothing at all and exit with 0.\n> The same problem with git whatchanged. The whole history mysteriously\n> disappeared at that commit, and git whatchanged exited with 0 without\n> any error or warning... Though git log will show you all history, but\n> if you run it with -p then it will also exit with zero at this commit\n> silently like previously history do not exist at all.\n\nIt turned out that git actually crashed while doing diff, but because it\nwas a forked process, the parent exited normally with code 0. I wonder\nshould not the parent process to exit with a non-zero code if the child\ndied by SIGSEG or another signal?\n\nDmitry\n"},{"id":"83520","messageId":"alpine.DEB.1.00.0807161546240.8950@racer","threadId":"14479","inReplyTo":"37fcd2780807160733o156e49c6m51314ff18764ccd5@mail.gmail.com","subject":"Re: [PATCH] Fix buffer overflow in git-grep","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-16T14:47:21Z","receivedAt":"2008-07-16T14:47:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 16 Jul 2008, Dmitry Potapov wrote:\n\n> On Wed, Jul 16, 2008 at 3:54 PM, Dmitry Potapov <dpotapov@gmail.com> wrote:\n> >\n> > For instance, git show will show you nothing at all and exit with 0.\n> > The same problem with git whatchanged. The whole history mysteriously\n> > disappeared at that commit, and git whatchanged exited with 0 without\n> > any error or warning... Though git log will show you all history, but\n> > if you run it with -p then it will also exit with zero at this commit\n> > silently like previously history do not exist at all.\n> \n> It turned out that git actually crashed while doing diff, but because it\n> was a forked process, the parent exited normally with code 0. I wonder\n> should not the parent process to exit with a non-zero code if the child\n> died by SIGSEG or another signal?\n\nWe'll probably need to use the MinGW pager handling for Unix, too, and \ncheck for a died child.\n\nNot overly trivial, though.\n\nCiao,\nDscho\n"},{"id":"83521","messageId":"1216220043-27678-1-git-send-email-dpotapov@gmail.com","threadId":"14479","inReplyTo":"37fcd2780807160733o156e49c6m51314ff18764ccd5@mail.gmail.com","subject":"[PATCH] Fix buffer overflow in git diff","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T14:54:02Z","receivedAt":"2008-07-16T14:54:02Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If PATH_MAX on your system is smaller than a path stored, it may cause\nbuffer overflow and stack corruption in diff_addremove() and diff_change()\nfunctions when running git-diff\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n diff-lib.c  |    8 ++++----\n diff.c      |   11 ++---------\n diff.h      |    9 ++++-----\n revision.c  |    4 ++--\n tree-diff.c |   26 ++++++++++++++++++++++----\n 5 files changed, 34 insertions(+), 24 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b17722d..e7eaff9 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -171,7 +171,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tif (silent_on_removed)\n \t\t\t\tcontinue;\n \t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n-\t\t\t\t       ce->sha1, ce->name, NULL);\n+\t\t\t\t       ce->sha1, ce->name);\n \t\t\tcontinue;\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n@@ -184,7 +184,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n \t\t\t    ce->sha1, (changed ? null_sha1 : ce->sha1),\n-\t\t\t    ce->name, NULL);\n+\t\t\t    ce->name);\n \n \t}\n \tdiffcore_std(&revs->diffopt);\n@@ -208,7 +208,7 @@ static void diff_index_show_file(struct rev_info *revs,\n \t\t\t\t const unsigned char *sha1, unsigned int mode)\n {\n \tdiff_addremove(&revs->diffopt, prefix[0], mode,\n-\t\t       sha1, ce->name, NULL);\n+\t\t       sha1, ce->name);\n }\n \n static int get_stat_data(struct cache_entry *ce,\n@@ -312,7 +312,7 @@ static int show_modified(struct oneway_unpack_data *cbdata,\n \t\treturn 0;\n \n \tdiff_change(&revs->diffopt, oldmode, mode,\n-\t\t    old->sha1, sha1, old->name, NULL);\n+\t\t    old->sha1, sha1, old->name);\n \treturn 0;\n }\n \ndiff --git a/diff.c b/diff.c\nindex 78c4d3a..386de82 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3356,9 +3356,8 @@ int diff_result_code(struct diff_options *opt, int status)\n void diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n-\t\t    const char *base, const char *path)\n+\t\t    const char *concatpath)\n {\n-\tchar concatpath[PATH_MAX];\n \tstruct diff_filespec *one, *two;\n \n \tif (DIFF_OPT_TST(options, IGNORE_SUBMODULES) && S_ISGITLINK(mode))\n@@ -3380,9 +3379,6 @@ void diff_addremove(struct diff_options *options,\n \t\taddremove = (addremove == '+' ? '-' :\n \t\t\t     addremove == '-' ? '+' : addremove);\n \n-\tif (!path) path = \"\";\n-\tsprintf(concatpath, \"%s%s\", base, path);\n-\n \tif (options->prefix &&\n \t    strncmp(concatpath, options->prefix, options->prefix_length))\n \t\treturn;\n@@ -3403,9 +3399,8 @@ void diff_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n-\t\t const char *base, const char *path)\n+\t\t const char *concatpath)\n {\n-\tchar concatpath[PATH_MAX];\n \tstruct diff_filespec *one, *two;\n \n \tif (DIFF_OPT_TST(options, IGNORE_SUBMODULES) && S_ISGITLINK(old_mode)\n@@ -3418,8 +3413,6 @@ void diff_change(struct diff_options *options,\n \t\ttmp = old_mode; old_mode = new_mode; new_mode = tmp;\n \t\ttmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;\n \t}\n-\tif (!path) path = \"\";\n-\tsprintf(concatpath, \"%s%s\", base, path);\n \n \tif (options->prefix &&\n \t    strncmp(concatpath, options->prefix, options->prefix_length))\ndiff --git a/diff.h b/diff.h\nindex 5dc0cb5..50fb5dd 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -14,12 +14,12 @@ typedef void (*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n-\t\t const char *base, const char *path);\n+\t\t const char *fullpath);\n \n typedef void (*add_remove_fn_t)(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n-\t\t    const char *base, const char *path);\n+\t\t    const char *fullpath);\n \n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n@@ -164,14 +164,13 @@ extern void diff_addremove(struct diff_options *,\n \t\t\t   int addremove,\n \t\t\t   unsigned mode,\n \t\t\t   const unsigned char *sha1,\n-\t\t\t   const char *base,\n-\t\t\t   const char *path);\n+\t\t\t   const char *fullpath);\n \n extern void diff_change(struct diff_options *,\n \t\t\tunsigned mode1, unsigned mode2,\n \t\t\tconst unsigned char *sha1,\n \t\t\tconst unsigned char *sha2,\n-\t\t\tconst char *base, const char *path);\n+\t\t\tconst char *fullpath);\n \n extern void diff_unmerge(struct diff_options *,\n \t\t\t const char *path,\ndiff --git a/revision.c b/revision.c\nindex fc66755..8dc3ca7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -259,7 +259,7 @@ static int tree_difference = REV_TREE_SAME;\n static void file_add_remove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n-\t\t    const char *base, const char *path)\n+\t\t    const char *fullpath)\n {\n \tint diff = REV_TREE_DIFFERENT;\n \n@@ -285,7 +285,7 @@ static void file_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n-\t\t const char *base, const char *path)\n+\t\t const char *fullpath)\n {\n \ttree_difference = REV_TREE_DIFFERENT;\n \tDIFF_OPT_SET(options, HAS_CHANGES);\ndiff --git a/tree-diff.c b/tree-diff.c\nindex e1e2e6c..4c67ea5 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -15,6 +15,15 @@ static char *malloc_base(const char *base, int baselen, const char *path, int pa\n \treturn newbase;\n }\n \n+static char *malloc_fullname(const char *base, int baselen, const char *path, int pathlen)\n+{\n+\tchar *fullname = xmalloc(baselen + pathlen + 1);\n+\tmemcpy(fullname, base, baselen);\n+\tmemcpy(fullname + baselen, path, pathlen);\n+\tfullname[baselen + pathlen] = 0;\n+\treturn fullname;\n+}\n+\n static void show_entry(struct diff_options *opt, const char *prefix, struct tree_desc *desc,\n \t\t       const char *base, int baselen);\n \n@@ -24,6 +33,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \tconst char *path1, *path2;\n \tconst unsigned char *sha1, *sha2;\n \tint cmp, pathlen1, pathlen2;\n+\tchar *fullname;\n \n \tsha1 = tree_entry_extract(t1, &path1, &mode1);\n \tsha2 = tree_entry_extract(t2, &path2, &mode2);\n@@ -56,14 +66,20 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \t\tint retval;\n \t\tchar *newbase = malloc_base(base, baselen, path1, pathlen1);\n \t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))\n+\t\t{\n+\t\t\tnewbase[baselen + pathlen1] = 0;\n \t\t\topt->change(opt, mode1, mode2,\n-\t\t\t\t    sha1, sha2, base, path1);\n+\t\t\t\t    sha1, sha2, newbase);\n+\t\t\tnewbase[baselen + pathlen1] = '/';\n+\t\t}\n \t\tretval = diff_tree_sha1(sha1, sha2, newbase, opt);\n \t\tfree(newbase);\n \t\treturn retval;\n \t}\n \n-\topt->change(opt, mode1, mode2, sha1, sha2, base, path1);\n+\tfullname = malloc_fullname(base, baselen, path1, pathlen1);\n+\topt->change(opt, mode1, mode2, sha1, sha2, fullname);\n+\tfree(fullname);\n \treturn 0;\n }\n \n@@ -205,10 +221,10 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \tunsigned mode;\n \tconst char *path;\n \tconst unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);\n+\tint pathlen = tree_entry_len(path, sha1);\n \n \tif (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {\n \t\tenum object_type type;\n-\t\tint pathlen = tree_entry_len(path, sha1);\n \t\tchar *newbase = malloc_base(base, baselen, path, pathlen);\n \t\tstruct tree_desc inner;\n \t\tvoid *tree;\n@@ -224,7 +240,9 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \t\tfree(tree);\n \t\tfree(newbase);\n \t} else {\n-\t\topt->add_remove(opt, prefix[0], mode, sha1, base, path);\n+\t\tchar *fullname = malloc_fullname(base, baselen, path, pathlen);\n+\t\topt->add_remove(opt, prefix[0], mode, sha1, fullname);\n+\t\tfree(fullname);\n \t}\n }\n \n-- \n1.5.6.3.3.gfcafb\n"},{"id":"83522","messageId":"1216220043-27678-2-git-send-email-dpotapov@gmail.com","threadId":"14479","inReplyTo":"1216220043-27678-1-git-send-email-dpotapov@gmail.com","subject":"[PATCH] Fix buffer overflow in prepare_attr_stack","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T14:54:03Z","receivedAt":"2008-07-16T14:54:03Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If PATH_MAX on your system is smaller than a path stored in the git repo,\nit may cause the buffer overflow in prepare_attr_stack.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n attr.c |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 0fb47d3..73b6d6d 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -459,7 +459,9 @@ static void prepare_attr_stack(const char *path, int dirlen)\n {\n \tstruct attr_stack *elem, *info;\n \tint len;\n-\tchar pathbuf[PATH_MAX];\n+\tstruct strbuf pathbuf;\n+\n+\tstrbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));\n \n \t/*\n \t * At the bottom of the attribute stack is the built-in\n@@ -510,13 +512,14 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t\t\tlen = strlen(attr_stack->origin);\n \t\t\tif (dirlen <= len)\n \t\t\t\tbreak;\n-\t\t\tmemcpy(pathbuf, path, dirlen);\n-\t\t\tmemcpy(pathbuf + dirlen, \"/\", 2);\n-\t\t\tcp = strchr(pathbuf + len + 1, '/');\n+\t\t\tpathbuf.len = 0;\n+\t\t\tstrbuf_add(&pathbuf, path, dirlen);\n+\t\t\tstrbuf_addch(&pathbuf, '/');\n+\t\t\tcp = strchr(pathbuf.buf + len + 1, '/');\n \t\t\tstrcpy(cp + 1, GITATTRIBUTES_FILE);\n-\t\t\telem = read_attr(pathbuf, 0);\n+\t\t\telem = read_attr(pathbuf.buf, 0);\n \t\t\t*cp = '\\0';\n-\t\t\telem->origin = strdup(pathbuf);\n+\t\t\telem->origin = strdup(pathbuf.buf);\n \t\t\telem->prev = attr_stack;\n \t\t\tattr_stack = elem;\n \t\t\tdebug_push(elem);\n-- \n1.5.6.3.3.gfcafb\n"},{"id":"83524","messageId":"487E11F7.60601@viscovery.net","threadId":"14479","inReplyTo":"1216220043-27678-2-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] Fix buffer overflow in prepare_attr_stack","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-07-16T15:21:27Z","receivedAt":"2008-07-16T15:21:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Dmitry Potapov schrieb:\n> +\t\t\tpathbuf.len = 0;\n\n+\t\t\tstrbuf_reset(&pathbuf);\n\n-- Hannes\n"},{"id":"83525","messageId":"1216222409-31785-1-git-send-email-dpotapov@gmail.com","threadId":"14479","inReplyTo":"alpine.DEB.1.00.0807161232110.8503@eeepc-johanness","subject":"[PATCH v2] Fix buffer overflow in git-grep","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T15:33:29Z","receivedAt":"2008-07-16T15:33:29Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If PATH_MAX on your system is smaller than any path stored in the git\nrepository, that can cause memory corruption inside of the grep_tree\nfunction used by git-grep.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\nI have converted grep_tree code to use path_buf.\n\n builtin-grep.c |   26 ++++++++++++++------------\n 1 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex ef29910..507bb95 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -427,33 +427,35 @@ static int grep_tree(struct grep_opt *opt, const char **paths,\n \tstruct name_entry entry;\n \tchar *down;\n \tint tn_len = strlen(tree_name);\n-\tchar *path_buf = xmalloc(PATH_MAX + tn_len + 100);\n+\tstruct strbuf pathbuf;\n+\n+\tstrbuf_init(&pathbuf, PATH_MAX + tn_len);\n \n \tif (tn_len) {\n-\t\ttn_len = sprintf(path_buf, \"%s:\", tree_name);\n-\t\tdown = path_buf + tn_len;\n-\t\tstrcat(down, base);\n-\t}\n-\telse {\n-\t\tdown = path_buf;\n-\t\tstrcpy(down, base);\n+\t\tstrbuf_add(&pathbuf, tree_name, tn_len);\n+\t\tstrbuf_addch(&pathbuf, ':');\n+\t\ttn_len = pathbuf.len;\n \t}\n-\tlen = strlen(path_buf);\n+\tstrbuf_addstr(&pathbuf, base);\n+\tlen = pathbuf.len;\n \n \twhile (tree_entry(tree, &entry)) {\n-\t\tstrcpy(path_buf + len, entry.path);\n+\t\tint te_len = tree_entry_len(entry.path, entry.sha1);\n+\t\tpathbuf.len = len;\n+\t\tstrbuf_add(&pathbuf, entry.path, te_len);\n \n \t\tif (S_ISDIR(entry.mode))\n \t\t\t/* Match \"abc/\" against pathspec to\n \t\t\t * decide if we want to descend into \"abc\"\n \t\t\t * directory.\n \t\t\t */\n-\t\t\tstrcpy(path_buf + len + tree_entry_len(entry.path, entry.sha1), \"/\");\n+\t\t\tstrbuf_addch(&pathbuf, '/');\n \n+\t\tdown = pathbuf.buf + tn_len;\n \t\tif (!pathspec_matches(paths, down))\n \t\t\t;\n \t\telse if (S_ISREG(entry.mode))\n-\t\t\thit |= grep_sha1(opt, entry.sha1, path_buf, tn_len);\n+\t\t\thit |= grep_sha1(opt, entry.sha1, pathbuf.buf, tn_len);\n \t\telse if (S_ISDIR(entry.mode)) {\n \t\t\tenum object_type type;\n \t\t\tstruct tree_desc sub;\n-- \n1.5.6.3.1.g4e6bb\n"},{"id":"83526","messageId":"20080716153955.GE2925@dpotapov.dyndns.org","threadId":"14479","inReplyTo":"487E11F7.60601@viscovery.net","subject":"[PATCH v2] Fix buffer overflow in prepare_attr_stack","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-07-16T15:39:55Z","receivedAt":"2008-07-16T15:39:55Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"If PATH_MAX on your system is smaller than a path stored in the git repo,\nit may cause the buffer overflow in prepare_attr_stack.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n\nOn Wed, Jul 16, 2008 at 05:21:27PM +0200, Johannes Sixt wrote:\n> Dmitry Potapov schrieb:\n> > +\t\t\tpathbuf.len = 0;\n> \n> +\t\t\tstrbuf_reset(&pathbuf);\n> \n> -- Hannes\n\n attr.c |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 0fb47d3..17f6a4d 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -459,7 +459,9 @@ static void prepare_attr_stack(const char *path, int dirlen)\n {\n \tstruct attr_stack *elem, *info;\n \tint len;\n-\tchar pathbuf[PATH_MAX];\n+\tstruct strbuf pathbuf;\n+\n+\tstrbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));\n \n \t/*\n \t * At the bottom of the attribute stack is the built-in\n@@ -510,13 +512,14 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t\t\tlen = strlen(attr_stack->origin);\n \t\t\tif (dirlen <= len)\n \t\t\t\tbreak;\n-\t\t\tmemcpy(pathbuf, path, dirlen);\n-\t\t\tmemcpy(pathbuf + dirlen, \"/\", 2);\n-\t\t\tcp = strchr(pathbuf + len + 1, '/');\n+\t\t\tstrbuf_reset(&pathbuf);\n+\t\t\tstrbuf_add(&pathbuf, path, dirlen);\n+\t\t\tstrbuf_addch(&pathbuf, '/');\n+\t\t\tcp = strchr(pathbuf.buf + len + 1, '/');\n \t\t\tstrcpy(cp + 1, GITATTRIBUTES_FILE);\n-\t\t\telem = read_attr(pathbuf, 0);\n+\t\t\telem = read_attr(pathbuf.buf, 0);\n \t\t\t*cp = '\\0';\n-\t\t\telem->origin = strdup(pathbuf);\n+\t\t\telem->origin = strdup(pathbuf.buf);\n \t\t\telem->prev = attr_stack;\n \t\t\tattr_stack = elem;\n \t\t\tdebug_push(elem);\n-- \n1.5.6.3.3.geccd\n"}]}