Re: [PATCH v4 12/24] read-cache: read index-v5

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Nov 30, 2013, 20:27 UTC
Message-ID: <87li05y6sq.fsf@gmail.com>
In-Reply-To: <CALWbr2xUMHSU0MV-6nVbN4_eSMoj3Eyc_Ta_CxTwZ_Y8tLfbdQ@mail.gmail.com>

Antoine Pelisse <apelisse@gmail.com> writes:

Show 27 quoted lines

> On Wed, Nov 27, 2013 at 1:00 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> +static int verify_hdr(void *mmap, unsigned long size)
>> +{
>> +       uint32_t *filecrc;
>> +       unsigned int header_size;
>> +       struct cache_header *hdr;
>> +       struct cache_header_v5 *hdr_v5;
>> +
>> +       if (size < sizeof(struct cache_header)
>> +           + sizeof (struct cache_header_v5) + 4)
>> +               die("index file smaller than expected");
>> +
>> +       hdr = mmap;
>> +       hdr_v5 = ptr_add(mmap, sizeof(*hdr));
>> +       /* Size of the header + the size of the extensionoffsets */
>> +       header_size = sizeof(*hdr) + sizeof(*hdr_v5) + hdr_v5->hdr_nextension * 4;
>> +       /* Initialize crc */
>> +       filecrc = ptr_add(mmap, header_size);
>> +       if (!check_crc32(0, hdr, header_size, ntohl(*filecrc)))
>> +               return error("bad index file header crc signature");
>> +       return 0;
>> +}
>
> I find it curious that we actually need a value from the header (and
> use it for pointer arithmetic) to check that the header is valid. The
> application will crash before the crc is checked if
> hdr_v5->hdr_nextensions is corrupted. Or am I missing something ?

Good catch, I'm the one that was missing something here. We still need to use the value from the header before calculating the crc, but should check if header_size - 4 is less than the total size of the index file. Then even if the header is corrupted we won't read anything that is not mmap'ed and thus won't crash.

This guard should also be included for everything else that checks the crc checksum, as that has the same problems and the calculated place in the file for the crc might be after the end of the file.

Thanks, will fix in the re-roll.