read-cache: fix index corruption with index v4
PATCH 1/1 read-cache: fix index corruption with index v4
ce012deb98 ("read-cache: avoid allocating every ondisk entry when writing", 2017-08-21) changed the way cache entries are written to the index file. While previously it wrote the name to an struct that was allocated using xcalloc(), it now uses ce_write() directly. Previously ce_namelen - common bytes were written to the cache entry, which would automatically make it nul terminated, as it was allocated using calloc.
Now we are writing ce_namelen - common + 1 bytes directly from the ce->name to the index. As ce->name is however not nul terminated, and index-v4 needs the nul terminator to split between one index entry and the next, this would end up in a corrupted index.
Fix that by only writing ce_namelen - common bytes directly from ce->name to the index, and adding the nul terminator in an extra call to ce_write.
This bug was turned up by setting TEST_GIT_INDEX_VERSION = 4 in config.mak and running the test suite (t1700 specifically broke).
---
I unfortunately didn't have more time to dig so
Quoted 1 line
As
ce->nameis however not nul terminated
just comes from my memory and from the patch below actually fixing the corruption, so it's really the most likely cause. Would be great if someone who can remember more about the index could confirm that this is indeed the case.
read-cache.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
read-cache.c +3 -1
@@ -2103,7 +2103,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,if (!result) result =ce_write (c, fd, to_remove_vi, prefix_size);if (!result) - result =ce_write (c, fd, ce->name + common,ce_namelen (ce) - common +1 ); + result =ce_write (c, fd, ce->name + common,ce_namelen (ce) - common); +if (!result) + result =ce_write (c, fd,"\0" ,1 );strbuf_splice (previous_name, common, to_remove, ce->name + common,ce_namelen (ce) - common);
Signature · 1 line
2.14.1.480.gb18f417b89
Quoted 25 lines
From: Thomas Gummerer [mailto:[email protected]]
Sent: Monday, September 4, 2017 4:58 PM
ce012deb98("read-cache: avoid allocating every ondisk entry when writing", 2017-08-21) changed the way cache entries are written to the index file. While previously it wrote the name to an struct that was allocated usingxcalloc(), it now usesce_write()directly. Previouslyce_namelen- common bytes were written to the cache entry, which would automatically make it nul terminated, as it was allocated using calloc.Now we are writing
ce_namelen- common + 1 bytes directly from thece->nameto the index. Asce->nameis however not nul terminated, and index-v4 needs the nul terminator to split between one index entry and the next, this would end up in a corrupted index.Fix that by only writing
ce_namelen- common bytes directly fromce->nameto the index, and adding the nul terminator in an extra call toce_write.This bug was turned up by setting
TEST_GIT_INDEX_VERSION= 4 in config.mak and running the test suite (t1700 specifically broke).Signed-off-by Thomas Gummerer <[email protected]>---
Quoted 9 lines
I unfortunately didn't have more time to dig so
Quoted 1 line
As
ce->nameis however not nul terminatedjust comes from my memory and from the patch below actually fixing the corruption, so it's really the most likely cause. Would be great if someone who can remember more about the index could confirm that this is indeed the case.
Digging into this and ce->name IS nul terminated. The issue comes in when the CE_STRIP_NAME is set, which is only set when using a split index. This sets the ce->ce_namelen = 0 without changing the actual ce->name buffer. When writing the entry for the split index version 4 it was using the first character
in the ce->name buffer because of the + 1, which obviously isn't correct. Before
it was using a newly allocated name buffer from the ondisk struct which was allocated based on the ce_namelen of zero.
Quoted 8 lines
read-cache.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)read-cache.c +0
-0
read-cache.c static int ce_write_entry(git_SHA_CTX *c, int fd, struct · line 2111
Quoted 10 lines
read-cache.c +3
-1@@ -2103,7 +2103,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce, if (!result) result = ce_write(c, fd, to_remove_vi, prefix_size);if (!result) - result =ce_write (c, fd, ce->name + common, ce_namelen(ce) - common + 1); + result =ce_write (c, fd, ce->name + common,ce_namelen (ce) - common); +if (!result) + result =ce_write (c, fd,"\0" ,1 );
You could use the padding variable here as well which is used in the < version 4 ce_write.
Quoted 5 lines
read-cache.c +0
-1strbuf_splice (previous_name, common, to_remove, ce->name + common,ce_namelen (ce) - common); --2.14.1.480.gb18f417b89
While looking at the code I was wondering if we could get around the whole setting ce->ce_namelen to zero bit but that would be much bigger patch and possibly introduce other bugs so this seems the appropriate fix for now.
Thanks for finding this!
Kevin
Quoted 2 lines · Kevin Willford
Quoted 2 lines
From: Thomas Gummerer [mailto:[email protected]]
Sent: Monday, September 4, 2017 4:58 PM
[..]
Quoted 10 lines
Quoted 9 lines
I unfortunately didn't have more time to dig so
Quoted 1 line
As
ce->nameis however not nul terminatedjust comes from my memory and from the patch below actually fixing the corruption, so it's really the most likely cause. Would be great if someone who can remember more about the index could confirm that this is indeed the case.
Quoted 7 lines
Digging into this and
ce->nameIS nul terminated. The issue comes in when theCE_STRIP_NAMEis set, which is only set when using a split index. This sets thece->ce_namelen= 0 without changing the actualce->namebuffer. When writing the entry for the split index version 4 it was using the first character
in thece->namebuffer because of the + 1, which obviously isn't correct. Before
it was using a newly allocated name buffer from the ondisk struct which was allocated based on thece_namelenof zero.
Thank you very much for digging into this. That also explains why only t1700 was
affected, but none of the other tests. Will update the commit message.
Quoted 13 lines
Quoted 13 lines
read-cache.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)read-cache.c +0
-1@@ -2103,7 +2103,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce, if (!result) result = ce_write(c, fd, to_remove_vi, prefix_size);if (!result) - result = ce_write(c, fd, ce->name + common,
Quoted 8 lines
read-cache.c static int ce_write_entry(git_SHA_CTX *c, int fd, struct · line 2111
Quoted 5 lines
read-cache.c +3
-0ce_namelen(ce) - common + 1); + result =ce_write (c, fd, ce->name + common,ce_namelen (ce) - common); +if (!result) + result =ce_write (c, fd,"\0" ,1 );You could use the padding variable here as well which is used in the < version 4
ce_write.
Thanks, will do that.
Quoted 6 lines
Quoted 5 lines
strbuf_splice(previous_name, common,
to_remove,
ce->name+ common,ce_namelen(ce)- common);
--
2.14.1.480.gb18f417b89
Quoted 6 lines
While looking at the code I was wondering if we could get around the whole setting
ce->ce_namelento zero bit but that would be much bigger patch and possibly introduce other bugs so this seems the appropriate fix for now.Thanks for finding this!
Thanks for the review! Will send an updated patch in a bit.
Quoted 1 line
Kevin
PATCH v2 1/1 read-cache: fix index corruption with index v4
ce012deb98 ("read-cache: avoid allocating every ondisk entry when writing", 2017-08-21) changed the way cache entries are written to the index file. While previously it wrote the name to an struct that was allocated using xcalloc(), it now uses ce_write() directly. Previously ce_namelen - common bytes were written to the cache entry, which would automatically make it nul terminated, as it was allocated using calloc.
Now we are writing ce_namelen - common + 1 bytes directly from the ce->name to the index. If CE_STRIP_NAME however gets set in the split index case ce->ce_namelen is set to 0 without changing the actual ce->name buffer. When index-v4, this results in the first character of ce->name being written out instead of just a terminating nul charcter.
As index-v4 requires the terminating nul character as terminator of the name when reading it back, this results in a corrupted index.
Fix that by only writing ce_namelen - common bytes directly from ce->name to the index, and adding the nul terminator in an extra call to ce_write.
This bug was turned up by setting TEST_GIT_INDEX_VERSION = 4 in config.mak and running the test suite (t1700 specifically broke).
---
Quoted 1 line
Will send an updated patch in a bit.
In a bit was a lie, I didn't get to it anymore yesterday, but here it is :)
read-cache.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
read-cache.c +3 -1
@@ -2103,7 +2103,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,if (!result) result =ce_write (c, fd, to_remove_vi, prefix_size);if (!result) - result =ce_write (c, fd, ce->name + common,ce_namelen (ce) - common +1 ); + result =ce_write (c, fd, ce->name + common,ce_namelen (ce) - common); +if (!result) + result =ce_write (c, fd, padding,1 );strbuf_splice (previous_name, common, to_remove, ce->name + common,ce_namelen (ce) - common);
Signature · 1 line
2.14.1.480.gb18f417b89