On Tue, Sep 5, 2017 at 8:37 PM, Kevin Willford <kewi...@microsoft.com> wrote: >> From: Thomas Gummerer [mailto:t.gumme...@gmail.com] >> Sent: Monday, September 4, 2017 4:58 PM
[..] >> I unfortunately didn't have more time to dig so >> >> > As ce->name is 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. >> > > 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. 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. >> read-cache.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/read-cache.c b/read-cache.c >> index 40da87ea71..80830ddcfc 100644 >> --- a/read-cache.c >> +++ b/read-cache.c >> @@ -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. Thanks, will do that. >> >> strbuf_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! Thanks for the review! Will send an updated patch in a bit. > Kevin
