Re: A possible divide by zero problem in read-cache.c

From: Jeff King <peff@peff.net>
Date: Apr 29, 2021, 20:56 UTC
Message-ID: <YIsdhdQX03osraFD@coredump.intra.peff.net>
In-Reply-To: <20210429145424.515208-1-matheus.bernardino@usp.br>

On Thu, Apr 29, 2021 at 11:54:24AM -0300, Matheus Tavares wrote:

Show 25 quoted lines

> > Step 1: (calling another function load_cache_entries_threaded with
> > nr_threads as an argument )  [ link:
> > https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2247
> > ]
> >       src_offset += load_cache_entries_threaded(istate, mmap,
> > mmap_size, nr_threads, ieot);
> 
> Hmm, this function call is guarded by an `if (ieot)` block:
> 
> 	if (ieot) {
> 		src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);
> 		free(ieot);
> 	} else {
> 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
> 	}
> 
> 
> And `ieot` will only get a non-NULL value if this previous assignment was
> executed:
> 
> 	if (extension_offset && nr_threads > 1)
> 		ieot = read_ieot_extension(mmap, mmap_size, extension_offset);
> 
> So it seems to me that we only call `load_cache_entries_threaded()` when
> `nr_threads > 1`.

Thanks for tracing this. I agree that this doesn't seem to be triggerable along that patch for this reason.

There is another assignment to nr_threads inside the function with the division:

          if (nr_threads > ieot->nr)
                  nr_threads = ieot->nr;
          CALLOC_ARRAY(data, nr_threads);
  
          offset = ieot_start = 0;
          ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);

Is it possible for ieot->nr to be 0? Almost certainly it would be a bug for something to have written it, but I wonder if a malicious file could trigger this.

Going back to read_ieot_extension(), I think the answer is "no". It does:

         /* extension size - version bytes / bytes per entry */
          nr = (extsize - sizeof(uint32_t)) / (sizeof(uint32_t) + sizeof(uint32_t));
          if (!nr) {
                  error("invalid number of IEOT entries %d", nr);
                  return NULL;
          }
          ieot = xmalloc(sizeof(struct index_entry_offset_table)
                         + (nr * sizeof(struct index_entry_offset)));
          ieot->nr = nr;

so it will reject such an extension.

So I think all is well here.

-Peff