Re: [PATCH v7 3/7] eoie: add End of Index Entry (EOIE) extension

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 1, 2018, 15:30 UTC
Message-ID: <CACsJy8CX0TwVydzmqsjHK+W7tcaDgRCgeU3Gmc-bA1Ecf=Yz6A@mail.gmail.com>
In-Reply-To: <20181001134556.33232-4-peartben@gmail.com>

On Mon, Oct 1, 2018 at 3:46 PM Ben Peart <peartben@gmail.com> wrote:

Show 5 quoted lines

> @@ -2479,6 +2491,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>         if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
>                 return -1;
>
> +       offset = lseek(newfd, 0, SEEK_CUR) + write_buffer_len;

Note, lseek() could in theory return -1 on error. Looking at the error code list in the man page it's pretty unlikely though, unless

Show 19 quoted lines

> +static size_t read_eoie_extension(const char *mmap, size_t mmap_size)
> +{
> +       /*
> +        * The end of index entries (EOIE) extension is guaranteed to be last
> +        * so that it can be found by scanning backwards from the EOF.
> +        *
> +        * "EOIE"
> +        * <4-byte length>
> +        * <4-byte offset>
> +        * <20-byte hash>
> +        */
> +       const char *index, *eoie;
> +       uint32_t extsize;
> +       size_t offset, src_offset;
> +       unsigned char hash[GIT_MAX_RAWSZ];
> +       git_hash_ctx c;
> +
> +       /* ensure we have an index big enough to contain an EOIE extension */
> +       if (mmap_size < sizeof(struct cache_header) + EOIE_SIZE_WITH_HEADER + the_hash_algo->rawsz)

Using sizeof() for on-disk structures could be dangerous because you don't know how much padding there could be (I'm not sure if it's actually specified in the C language spec). I've checked, on at least x86 and amd64, sizeof(struct cache_header) is 12 bytes, but I don't know if there are any crazy architectures out there that set higher padding.

-- 
Duy