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

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

[Yiyuan guo]

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

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

[Matheus Tavares]

On Thu, Apr 29, 2021 at 11:33 AM Yiyuan guo <yguoaz@gmail.com> wrote:
>
> 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);

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

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

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

[Jeff King]

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

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