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
