# A possible divide by zero problem in read-cache.c 3 messages from 2021-04-29 to 2021-04-29. Participants: Yiyuan guo, Matheus Tavares, Jeff King. Thread: https://gitlist.dev/t/55588 ## Yiyuan guo, 2021-04-29 14:33 Subject: A possible divide by zero problem in read-cache.c Message-ID: URL: https://gitlist.dev/e/CAM7%3DBFooqCo3vn%2Byy6uM26szZDAcbx8M%3DsZS1znucJvHRbtxHg%40mail.gmail.com ``` 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.) ``` ## Matheus Tavares, 2021-04-29 14:54 Subject: Re: A possible divide by zero problem in read-cache.c Message-ID: <20210429145424.515208-1-matheus.bernardino@usp.br> URL: https://gitlist.dev/e/20210429145424.515208-1-matheus.bernardino%40usp.br In-Reply-To: ``` On Thu, Apr 29, 2021 at 11:33 AM Yiyuan guo 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); ``` ## Jeff King, 2021-04-29 20:56 Subject: Re: A possible divide by zero problem in read-cache.c Message-ID: URL: https://gitlist.dev/e/YIsdhdQX03osraFD%40coredump.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: > > 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 ```