name-hash: don't reuse cache_entry in dir_entry

Stop reusing cache_entry in dir_entry; doing so causes a
use-after-free bug.

During merges, we free entries that we no longer need in the
destination index.  But those entries might have also been stored in
the dir_entry cache, and when a later call to add_to_index found them,
they would be used after being freed.

To prevent this, change dir_entry to store a copy of the name instead
of a pointer to a cache_entry.  This entails some refactoring of code
that expects the cache_entry.

Keith McGuigan <kmcguigan@twitter.com> diagnosed this bug and wrote
the initial patch, but this version does not use any of Keith's code.

Helped-by: Keith McGuigan <kmcguigan@twitter.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index 22b7b8129072f3905c68f02fe19f463bdb738b9e..1afe5374b9f2bf4436c80209c219b3c6beb3c515 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -501,7 +501,8 @@ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsi
 extern int discard_index(struct index_state *);
 extern int unmerged_index(const struct index_state *);
 extern int verify_path(const char *path);
-extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen);
+extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
+extern void adjust_dirname_case(struct index_state *istate, char *name);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
 extern int index_name_pos(const struct index_state *, const char *name, int namelen);
 #define ADD_CACHE_OK_TO_ADD 1          /* Ok to add */

diff --git a/dir.c b/dir.c
index 3f7a0256b61552c6c3f80f710bd042b697d79da9..8545922ee2ddb5a218e173d938975c47bf76b335 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -964,29 +964,15 @@ enum exist_status {
  */
 static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)
 {
-       const struct cache_entry *ce = cache_dir_exists(dirname, len);
-       unsigned char endchar;
+       struct cache_entry *ce;
 
-       if (!ce)
-               return index_nonexistent;
-       endchar = ce->name[len];
-
-       /*
-        * The cache_entry structure returned will contain this dirname
-        * and possibly additional path components.
-        */
-       if (endchar == '/')
+       if (cache_dir_exists(dirname, len))
                return index_directory;
 
-       /*
-        * If there are no additional path components, then this cache_entry
-        * represents a submodule.  Submodules, despite being directories,
-        * are stored in the cache without a closing slash.
-        */
-       if (!endchar && S_ISGITLINK(ce->ce_mode))
+       ce = cache_file_exists(dirname, len, ignore_case);
+       if (ce && S_ISGITLINK(ce->ce_mode))
                return index_gitdir;
 
-       /* This should never be hit, but it exists just in case. */
        return index_nonexistent;
 }
 

diff --git a/name-hash.c b/name-hash.c
index 702cd0518fca67a84417de8268ed70dfa29392e4..332ba956e7edec6c6fbe10f346ec08fc469b846f 100644 (file)
--- a/name-hash.c
+++ b/name-hash.c
@@ -11,16 +11,16 @@
 struct dir_entry {
        struct hashmap_entry ent;
        struct dir_entry *parent;
-       struct cache_entry *ce;
        int nr;
        unsigned int namelen;
+       char name[FLEX_ARRAY];
 };
 
 static int dir_entry_cmp(const struct dir_entry *e1,
                const struct dir_entry *e2, const char *name)
 {
-       return e1->namelen != e2->namelen || strncasecmp(e1->ce->name,
-                       name ? name : e2->ce->name, e1->namelen);
+       return e1->namelen != e2->namelen || strncasecmp(e1->name,
+                       name ? name : e2->name, e1->namelen);
 }
 
 static struct dir_entry *find_dir_entry(struct index_state *istate,
@@ -41,14 +41,6 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,
         * closing slash.  Despite submodules being a directory, they never
         * reach this point, because they are stored
         * in index_state.name_hash (as ordinary cache_entries).
-        *
-        * Note that the cache_entry stored with the dir_entry merely
-        * supplies the name of the directory (up to dir_entry.namelen). We
-        * track the number of 'active' files in a directory in dir_entry.nr,
-        * so we can tell if the directory is still relevant, e.g. for git
-        * status. However, if cache_entries are removed, we cannot pinpoint
-        * an exact cache_entry that's still active. It is very possible that
-        * multiple dir_entries point to the same cache_entry.
         */
        struct dir_entry *dir;
 
@@ -63,10 +55,10 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,
        dir = find_dir_entry(istate, ce->name, namelen);
        if (!dir) {
                /* not found, create it and add to hash table */
-               dir = xcalloc(1, sizeof(struct dir_entry));
+               dir = xcalloc(1, sizeof(struct dir_entry) + namelen + 1);
                hashmap_entry_init(dir, memihash(ce->name, namelen));
                dir->namelen = namelen;
-               dir->ce = ce;
+               strncpy(dir->name, ce->name, namelen);
                hashmap_add(&istate->dir_hash, dir);
 
                /* recursively add missing parent directories */
@@ -188,26 +180,36 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
        return slow_same_name(name, namelen, ce->name, len);
 }
 
-struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen)
+int index_dir_exists(struct index_state *istate, const char *name, int namelen)
 {
-       struct cache_entry *ce;
        struct dir_entry *dir;
 
        lazy_init_name_hash(istate);
        dir = find_dir_entry(istate, name, namelen);
-       if (dir && dir->nr)
-               return dir->ce;
+       return dir && dir->nr;
+}
 
-       /*
-        * It might be a submodule. Unlike plain directories, which are stored
-        * in the dir-hash, submodules are stored in the name-hash, so check
-        * there, as well.
-        */
-       ce = index_file_exists(istate, name, namelen, 1);
-       if (ce && S_ISGITLINK(ce->ce_mode))
-               return ce;
+void adjust_dirname_case(struct index_state *istate, char *name)
+{
+       const char *startPtr = name;
+       const char *ptr = startPtr;
 
-       return NULL;
+       lazy_init_name_hash(istate);
+       while (*ptr) {
+               while (*ptr && *ptr != '/')
+                       ptr++;
+
+               if (*ptr == '/') {
+                       struct dir_entry *dir;
+
+                       ptr++;
+                       dir = find_dir_entry(istate, name, ptr - name + 1);
+                       if (dir) {
+                               memcpy((void *)startPtr, dir->name + (startPtr - name), ptr - startPtr);
+                               startPtr = ptr;
+                       }
+               }
+       }
 }
 
 struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int icase)

diff --git a/read-cache.c b/read-cache.c
index 9cff715d6b669ecf05f861bc553b61d20d04191d..3c388c35aea1652af196b0a1e9aa630822809b90 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -661,21 +661,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
         * entry's directory case.
         */
        if (ignore_case) {
-               const char *startPtr = ce->name;
-               const char *ptr = startPtr;
-               while (*ptr) {
-                       while (*ptr && *ptr != '/')
-                               ++ptr;
-                       if (*ptr == '/') {
-                               struct cache_entry *foundce;
-                               ++ptr;
-                               foundce = index_dir_exists(istate, ce->name, ptr - ce->name - 1);
-                               if (foundce) {
-                                       memcpy((void *)startPtr, foundce->name + (startPtr - ce->name), ptr - startPtr);
-                                       startPtr = ptr;
-                               }
-                       }
-               }
+               adjust_dirname_case(istate, ce->name);
        }
 
        alias = index_file_exists(istate, ce->name, ce_namelen(ce), ignore_case);