* A possible divide by zero problem in read-cache.c @ 2021-04-29 14:33 Yiyuan guo 2021-04-29 14:54 ` Matheus Tavares 0 siblings, 1 reply; 3+ messages in thread From: Yiyuan guo @ 2021-04-29 14:33 UTC (permalink / raw) To: git 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.) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: A possible divide by zero problem in read-cache.c 2021-04-29 14:33 A possible divide by zero problem in read-cache.c Yiyuan guo @ 2021-04-29 14:54 ` Matheus Tavares 2021-04-29 20:56 ` Jeff King 0 siblings, 1 reply; 3+ messages in thread From: Matheus Tavares @ 2021-04-29 14:54 UTC (permalink / raw) To: yguoaz; +Cc: git 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); ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: A possible divide by zero problem in read-cache.c 2021-04-29 14:54 ` Matheus Tavares @ 2021-04-29 20:56 ` Jeff King 0 siblings, 0 replies; 3+ messages in thread From: Jeff King @ 2021-04-29 20:56 UTC (permalink / raw) To: Matheus Tavares; +Cc: yguoaz, git 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-29 20:56 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-29 14:33 A possible divide by zero problem in read-cache.c Yiyuan guo 2021-04-29 14:54 ` Matheus Tavares 2021-04-29 20:56 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).