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

Hello, git developers. I have found a possible divide by zero problem in read-cache.c. Here is the trace (with links to code location) for triggering the bug:

Step 0: (In function do_read_index) [ link:
https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2216
]
      nr_threads = istate->cache_nr / THREAD_COST;
If istate->cache_nr == 0, nr_threads will also obtain 0 value.

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);

Step 2:  (use nr_threads as divisor, leading to possible divide by
zero in function load_cache_entries_threaded) [ link:
https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2103
]
      ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);

Please let me know if you think this bug report is genuine and worth fixing.

Thanks, Yiyuan

(PS: this report is originally sent to the security mailing list. After some discussions, it seems that it is more appropriate to post it in the public list, considering its threat level.)

On Thu, Apr 29, 2021 at 11:33 AM Yiyuan guo <yguoaz@gmail.com> wrote:

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`.

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

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