{"thread":{"id":"40611","subject":"[PATCH v4] name-hash: don't reuse cache_entry in dir_entry","startedAt":"2015-10-21T17:54:11Z","lastAt":"2015-10-21T19:49:04Z","messageCount":2,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"272046","messageId":"1445450051-7430-1-git-send-email-dturner@twopensource.com","threadId":"40611","inReplyTo":null,"subject":"[PATCH v4] name-hash: don't reuse cache_entry in dir_entry","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-10-21T17:54:11Z","receivedAt":"2015-10-21T17:54:11Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Stop reusing cache_entry in dir_entry; doing so causes a\nuse-after-free bug.\n\nDuring merges, we free entries that we no longer need in the\ndestination index.  But those entries might have also been stored in\nthe dir_entry cache, and when a later call to add_to_index found them,\nthey would be used after being freed.\n\nTo prevent this, change dir_entry to store a copy of the name instead\nof a pointer to a cache_entry.  This entails some refactoring of code\nthat expects the cache_entry.\n\nKeith McGuigan <kmcguigan@twitter.com> diagnosed this bug and wrote\nthe initial patch, but this version does not use any of Keith's code.\n\nHelped-by: Keith McGuigan <kmcguigan@twitter.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n cache.h      |  3 ++-\n dir.c        | 22 ++++------------------\n name-hash.c  | 54 ++++++++++++++++++++++++++++--------------------------\n read-cache.c | 16 +---------------\n 4 files changed, 35 insertions(+), 60 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 752031e..ccc329a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -520,7 +520,8 @@ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsi\n extern int discard_index(struct index_state *);\n extern int unmerged_index(const struct index_state *);\n extern int verify_path(const char *path);\n-extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen);\n+extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);\n+extern void adjust_dirname_case(struct index_state *istate, char *name);\n extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);\n extern int index_name_pos(const struct index_state *, const char *name, int namelen);\n #define ADD_CACHE_OK_TO_ADD 1\t\t/* Ok to add */\ndiff --git a/dir.c b/dir.c\nindex b90484a..c982aac 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1279,29 +1279,15 @@ enum exist_status {\n  */\n static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)\n {\n-\tconst struct cache_entry *ce = cache_dir_exists(dirname, len);\n-\tunsigned char endchar;\n+\tstruct cache_entry *ce;\n \n-\tif (!ce)\n-\t\treturn index_nonexistent;\n-\tendchar = ce->name[len];\n-\n-\t/*\n-\t * The cache_entry structure returned will contain this dirname\n-\t * and possibly additional path components.\n-\t */\n-\tif (endchar == '/')\n+\tif (cache_dir_exists(dirname, len))\n \t\treturn index_directory;\n \n-\t/*\n-\t * If there are no additional path components, then this cache_entry\n-\t * represents a submodule.  Submodules, despite being directories,\n-\t * are stored in the cache without a closing slash.\n-\t */\n-\tif (!endchar && S_ISGITLINK(ce->ce_mode))\n+\tce = cache_file_exists(dirname, len, ignore_case);\n+\tif (ce && S_ISGITLINK(ce->ce_mode))\n \t\treturn index_gitdir;\n \n-\t/* This should never be hit, but it exists just in case. */\n \treturn index_nonexistent;\n }\n \ndiff --git a/name-hash.c b/name-hash.c\nindex 702cd05..332ba95 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -11,16 +11,16 @@\n struct dir_entry {\n \tstruct hashmap_entry ent;\n \tstruct dir_entry *parent;\n-\tstruct cache_entry *ce;\n \tint nr;\n \tunsigned int namelen;\n+\tchar name[FLEX_ARRAY];\n };\n \n static int dir_entry_cmp(const struct dir_entry *e1,\n \t\tconst struct dir_entry *e2, const char *name)\n {\n-\treturn e1->namelen != e2->namelen || strncasecmp(e1->ce->name,\n-\t\t\tname ? name : e2->ce->name, e1->namelen);\n+\treturn e1->namelen != e2->namelen || strncasecmp(e1->name,\n+\t\t\tname ? name : e2->name, e1->namelen);\n }\n \n static struct dir_entry *find_dir_entry(struct index_state *istate,\n@@ -41,14 +41,6 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,\n \t * closing slash.  Despite submodules being a directory, they never\n \t * reach this point, because they are stored\n \t * in index_state.name_hash (as ordinary cache_entries).\n-\t *\n-\t * Note that the cache_entry stored with the dir_entry merely\n-\t * supplies the name of the directory (up to dir_entry.namelen). We\n-\t * track the number of 'active' files in a directory in dir_entry.nr,\n-\t * so we can tell if the directory is still relevant, e.g. for git\n-\t * status. However, if cache_entries are removed, we cannot pinpoint\n-\t * an exact cache_entry that's still active. It is very possible that\n-\t * multiple dir_entries point to the same cache_entry.\n \t */\n \tstruct dir_entry *dir;\n \n@@ -63,10 +55,10 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,\n \tdir = find_dir_entry(istate, ce->name, namelen);\n \tif (!dir) {\n \t\t/* not found, create it and add to hash table */\n-\t\tdir = xcalloc(1, sizeof(struct dir_entry));\n+\t\tdir = xcalloc(1, sizeof(struct dir_entry) + namelen + 1);\n \t\thashmap_entry_init(dir, memihash(ce->name, namelen));\n \t\tdir->namelen = namelen;\n-\t\tdir->ce = ce;\n+\t\tstrncpy(dir->name, ce->name, namelen);\n \t\thashmap_add(&istate->dir_hash, dir);\n \n \t\t/* recursively add missing parent directories */\n@@ -188,26 +180,36 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen\n \treturn slow_same_name(name, namelen, ce->name, len);\n }\n \n-struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen)\n+int index_dir_exists(struct index_state *istate, const char *name, int namelen)\n {\n-\tstruct cache_entry *ce;\n \tstruct dir_entry *dir;\n \n \tlazy_init_name_hash(istate);\n \tdir = find_dir_entry(istate, name, namelen);\n-\tif (dir && dir->nr)\n-\t\treturn dir->ce;\n+\treturn dir && dir->nr;\n+}\n \n-\t/*\n-\t * It might be a submodule. Unlike plain directories, which are stored\n-\t * in the dir-hash, submodules are stored in the name-hash, so check\n-\t * there, as well.\n-\t */\n-\tce = index_file_exists(istate, name, namelen, 1);\n-\tif (ce && S_ISGITLINK(ce->ce_mode))\n-\t\treturn ce;\n+void adjust_dirname_case(struct index_state *istate, char *name)\n+{\n+\tconst char *startPtr = name;\n+\tconst char *ptr = startPtr;\n \n-\treturn NULL;\n+\tlazy_init_name_hash(istate);\n+\twhile (*ptr) {\n+\t\twhile (*ptr && *ptr != '/')\n+\t\t\tptr++;\n+\n+\t\tif (*ptr == '/') {\n+\t\t\tstruct dir_entry *dir;\n+\n+\t\t\tptr++;\n+\t\t\tdir = find_dir_entry(istate, name, ptr - name + 1);\n+\t\t\tif (dir) {\n+\t\t\t\tmemcpy((void *)startPtr, dir->name + (startPtr - name), ptr - startPtr);\n+\t\t\t\tstartPtr = ptr;\n+\t\t\t}\n+\t\t}\n+\t}\n }\n \n struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int icase)\ndiff --git a/read-cache.c b/read-cache.c\nindex 87204a5..de22df2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -679,21 +679,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t * entry's directory case.\n \t */\n \tif (ignore_case) {\n-\t\tconst char *startPtr = ce->name;\n-\t\tconst char *ptr = startPtr;\n-\t\twhile (*ptr) {\n-\t\t\twhile (*ptr && *ptr != '/')\n-\t\t\t\t++ptr;\n-\t\t\tif (*ptr == '/') {\n-\t\t\t\tstruct cache_entry *foundce;\n-\t\t\t\t++ptr;\n-\t\t\t\tfoundce = index_dir_exists(istate, ce->name, ptr - ce->name - 1);\n-\t\t\t\tif (foundce) {\n-\t\t\t\t\tmemcpy((void *)startPtr, foundce->name + (startPtr - ce->name), ptr - startPtr);\n-\t\t\t\t\tstartPtr = ptr;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n+\t\tadjust_dirname_case(istate, ce->name);\n \t}\n \n \talias = index_file_exists(istate, ce->name, ce_namelen(ce), ignore_case);\n-- \n2.4.2.644.g97b850b-twtrsrc\n"},{"id":"272056","messageId":"xmqqpp08uhn3.fsf@gitster.mtv.corp.google.com","threadId":"40611","inReplyTo":"1445450051-7430-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v4] name-hash: don't reuse cache_entry in dir_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-21T19:49:04Z","receivedAt":"2015-10-21T19:49:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> Stop reusing cache_entry in dir_entry; doing so causes a\n> use-after-free bug.\n>\n> During merges, we free entries that we no longer need in the\n> destination index.  But those entries might have also been stored in\n> the dir_entry cache, and when a later call to add_to_index found them,\n> they would be used after being freed.\n>\n> To prevent this, change dir_entry to store a copy of the name instead\n> of a pointer to a cache_entry.  This entails some refactoring of code\n> that expects the cache_entry.\n>\n> Keith McGuigan <kmcguigan@twitter.com> diagnosed this bug and wrote\n> the initial patch, but this version does not use any of Keith's code.\n>\n> Helped-by: Keith McGuigan <kmcguigan@twitter.com>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n\nThe patch looks good to me.  Will replace the ce-refcnt one with\nthis.\n\nThanks for following it through.\n"}]}