Re: [PATCH v5] Verify index file before we opportunistically update it

On Thu, Apr 10, 2014 at 12:34 PM, Yiannis Marangos
[off-list ref] wrote:

+/*
+ * This function verifies if index_state has the correct sha1 of an index file.
+ * Don't die if we have any other failure, just return 0.
+ */
+static int verify_index_from(const struct index_state *istate, const char *path)
+{
+       int fd;
+       struct stat st;
+       struct cache_header *hdr;
+       void *mmap_addr;
+       size_t mmap_size;
+
+       if (!istate->initialized)
+               return 0;
+
+       fd = open(path, O_RDONLY);
+       if (fd < 0)
+               return 0;
+
+       if (fstat(fd, &st))
+               return 0;
+
+       /* file is too big */
+       if (st.st_size > (size_t)st.st_size)
+               return 0;
+
+       mmap_size = (size_t)st.st_size;
+       if (mmap_size < sizeof(struct cache_header) + 20)
+               return 0;
+
+       mmap_addr = mmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
+       close(fd);
+       if (mmap_addr == MAP_FAILED)
+               return 0;
+
+       hdr = mmap_addr;
+       if (verify_hdr(hdr, mmap_size) < 0)
+               goto unmap;

verify_hdr() is a bit expensive because you need to digest the whole
index file (could big as big as 14MB on webkit). Could we get away
without it? I mean, is it enough that we pick the last 20 bytes and
compare it with istate->sha1? If we only need 20 bytes, pread() may be
better than mmap().

The chance of SHA-1 collision is small enough for us to ignore, I
think. And if a client updates the index without updating the trailing
sha-1, the index is broken and we don't have to worry about
overwriting it.

+
+       if (hashcmp(istate->sha1, (unsigned char *)hdr + mmap_size - 20))
+               goto unmap;
+
+       munmap(mmap_addr, mmap_size);
+       return 1;
+
+unmap:
+       munmap(mmap_addr, mmap_size);
+       return 0;
+}

-- 
Duy