index: be careful when handling long names

We currently use lower 12-bit (masked with CE_NAMEMASK) in the
ce_flags field to store the length of the name in cache_entry,
without checking the length parameter given to
create_ce_flags().  This can make us store incorrect length.

Currently we are mostly protected by the fact that many
codepaths first copy the path in a variable of size PATH_MAX,
which typically is 4096 that happens to match the limit, but
that feels like a bug waiting to happen.  Besides, that would
not allow us to shorten the width of CE_NAMEMASK to use the bits
for new flags.

This redefines the meaning of the name length stored in the
cache_entry.  A name that does not fit is represented by storing
CE_NAMEMASK in the field, and the actual length needs to be
computed by actually counting the bytes in the name[] field.
This way, only the unusually long paths need to suffer.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/cache.h b/cache.h
index 4a054c5402c0e25770282bfae25b2a63cf3ca4b6..9eaffdefd03f9d8454540f2300992994bee27d86 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -131,8 +131,21 @@ struct cache_entry {
 #define CE_UPDATE    (0x10000)
 #define CE_REMOVE    (0x20000)
 
 #define CE_UPDATE    (0x10000)
 #define CE_REMOVE    (0x20000)
 

-#define create_ce_flags(len, stage) ((len) | ((stage) << CE_STAGESHIFT))
-#define ce_namelen(ce) (CE_NAMEMASK & (ce)->ce_flags)
+static inline unsigned create_ce_flags(size_t len, unsigned stage)
+{
+       if (len >= CE_NAMEMASK)
+               len = CE_NAMEMASK;
+       return (len | (stage << CE_STAGESHIFT));
+}
+
+static inline size_t ce_namelen(const struct cache_entry *ce)
+{
+       size_t len = ce->ce_flags & CE_NAMEMASK;
+       if (len < CE_NAMEMASK)
+               return len;
+       return strlen(ce->name + CE_NAMEMASK) + CE_NAMEMASK;
+}
+

 #define ce_size(ce) cache_entry_size(ce_namelen(ce))
 #define ondisk_ce_size(ce) ondisk_cache_entry_size(ce_namelen(ce))
 #define ce_stage(ce) ((CE_STAGEMASK & (ce)->ce_flags) >> CE_STAGESHIFT)
 #define ce_size(ce) cache_entry_size(ce_namelen(ce))
 #define ondisk_ce_size(ce) ondisk_cache_entry_size(ce_namelen(ce))
 #define ce_stage(ce) ((CE_STAGEMASK & (ce)->ce_flags) >> CE_STAGESHIFT)

diff --git a/read-cache.c b/read-cache.c
index 82a6238b7783ef41896834dae11a528e77f4e447..528f697f5995f360c49651338af94bc23e00d032 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -928,6 +928,8 @@ int read_index(struct index_state *istate)
 
 static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_entry *ce)
 {
 
 static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_entry *ce)
 {

+       size_t len;
+

        ce->ce_ctime = ntohl(ondisk->ctime.sec);
        ce->ce_mtime = ntohl(ondisk->mtime.sec);
        ce->ce_dev   = ntohl(ondisk->dev);
        ce->ce_ctime = ntohl(ondisk->ctime.sec);
        ce->ce_mtime = ntohl(ondisk->mtime.sec);
        ce->ce_dev   = ntohl(ondisk->dev);


@@ -939,7 +941,15 @@ static void convert_from_disk(struct ondisk_cache_entry *ondisk, struct cache_en
        /* On-disk flags are just 16 bits */
        ce->ce_flags = ntohs(ondisk->flags);
        hashcpy(ce->sha1, ondisk->sha1);
        /* On-disk flags are just 16 bits */
        ce->ce_flags = ntohs(ondisk->flags);
        hashcpy(ce->sha1, ondisk->sha1);

-       memcpy(ce->name, ondisk->name, ce_namelen(ce)+1);
+
+       len = ce->ce_flags & CE_NAMEMASK;
+       if (len == CE_NAMEMASK)
+               len = strlen(ondisk->name);
+       /*
+        * NEEDSWORK: If the original index is crafted, this copy could
+        * go unchecked.
+        */
+       memcpy(ce->name, ondisk->name, len + 1);

 }
 
 /* remember to discard_cache() before reading a different cache! */
 }
 
 /* remember to discard_cache() before reading a different cache! */

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 4e49d590651363631a1f3a795d3c1679a70fb05f..9f84b8d3acf198aafbd0d2a8445dce7ce976ee3d 100755 (executable)
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -297,4 +297,24 @@ test_expect_success 'absolute path works as expected' '
        test "$sym" = "$(test-absolute-path $dir2/syml)"
 '
 
        test "$sym" = "$(test-absolute-path $dir2/syml)"
 '
 

+test_expect_success 'very long name in the index handled sanely' '
+
+       a=a && # 1
+       a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 16
+       a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 256
+       a=$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a$a && # 4096
+       a=${a}q &&
+
+       >path4 &&
+       git update-index --add path4 &&
+       (
+               git ls-files -s path4 |
+               sed -e "s/      .*/     /" |
+               tr -d "\012"
+               echo "$a"
+       ) | git update-index --index-info &&
+       len=$(git ls-files "a*" | wc -c) &&
+       test $len = 4098
+'
+

 test_done
 test_done