From: Jakub Narebski <jnareb@gmail.com>
Date: Sat, 02 Jun 2018 22:50:10 GMT
Subject: Re: [PATCH v3 11/20] commit-graph: verify root tree OIDs
Message-ID: <86zi0cpzvh.fsf@gmail.com>
In-Reply-To: <d59f5723-7fb6-56d6-af2b-b8263cfdf354@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:
> On 5/30/2018 6:24 PM, Jakub Narebski wrote:

[...]
>> NOTE: we will be checking Commit Data chunk; I think it would be good
>> idea to verify that size of Commit Data chunk matches (N * (H + 16) bytes)
>> that format gives us, so that we don't accidentally red outside of
>> memory if something got screwed up (like number of commits being wrong,
>> or file truncated).
>
> This is actually how we calculate 'num_commits' during
> load_commit_graph_one():
>
>     if (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)
>     {
>         graph->num_commits = (chunk_offset - last_chunk_offset)
>                              / graph->hash_len;
>     }
>
> So, if the chunk doesn't match N*(H+16), we detect this because
> FANOUT[255] != N.
>
> (There is one caveat here: (chunk_offset - last_chunk_offset) may not
> be a multiple of hash_len, and "accidentally" truncate to N in the
> division. I'll add more careful checks for this.)

I have thought for some reason that number of commits N was stored
somewhere directly in the commit-graph header.

Anyway we have three places that we can calculate (or simply read in
case of OID Fanour chunk) the number of commits:
 - FANOUT[255] == N
 - OID Lookup size = (N * H bytes)
   - N = (OID Lookup size) / hash_len
   - (OID Lookup size) % hash_len == 0
 - Commit Data size = (N * (H + 16) bytes)
   - N = (Commir Data size) / (hash_len + 16)
   - (Commir Data size) % (hash_len + 16) == 0

>
> We also check out-of-bounds offsets in that method.

Good.

Best,
-- 
Jakub Narębski