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

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 10, 2014, 10:40 UTC
Message-ID: <CACsJy8D9dsxOA6xVmhxa7N20OxEQ1a63rQm6QCtwq9ThG6E3RA@mail.gmail.com>
In-Reply-To: <1397108075-30891-1-git-send-email-yiannis.marangos@gmail.com>

On Thu, Apr 10, 2014 at 12:34 PM, Yiannis Marangos <yiannis.marangos@gmail.com> wrote:

Show 38 quoted lines

> +/*
> + * 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.

Show 11 quoted lines

> +
> +       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