{"thread":{"id":"55588","subject":"A possible divide by zero problem in read-cache.c","startedAt":"2021-04-29T14:33:22Z","lastAt":"2021-04-29T20:56:39Z","messageCount":3,"participants":["Yiyuan guo","Matheus Tavares","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"423271","messageId":"CAM7=BFooqCo3vn+yy6uM26szZDAcbx8M=sZS1znucJvHRbtxHg@mail.gmail.com","threadId":"55588","inReplyTo":null,"subject":"A possible divide by zero problem in read-cache.c","fromName":"Yiyuan guo","fromEmail":"yguoaz@gmail.com","sentAt":"2021-04-29T14:33:05Z","receivedAt":"2021-04-29T14:33:22Z","isPatch":false,"sender":{"key":"yguoaz@gmail.com","avatar":null},"body":"Hello, git developers.\nI have found a possible divide by zero problem in read-cache.c. Here\nis the trace (with links to code location) for triggering the bug:\n\nStep 0: (In function do_read_index) [ link:\nhttps://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2216\n]\n      nr_threads = istate->cache_nr / THREAD_COST;\nIf istate->cache_nr == 0, nr_threads will also obtain 0 value.\n\nStep 1: (calling another function load_cache_entries_threaded with\nnr_threads as an argument )  [ link:\nhttps://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2247\n]\n      src_offset += load_cache_entries_threaded(istate, mmap,\nmmap_size, nr_threads, ieot);\n\nStep 2:  (use nr_threads as divisor, leading to possible divide by\nzero in function load_cache_entries_threaded) [ link:\nhttps://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2103\n]\n      ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);\n\nPlease let me know if you think this bug report is genuine and worth fixing.\n\nThanks, Yiyuan\n\n(PS: this report is originally sent to the security mailing list.\nAfter some discussions, it seems that it is more appropriate to post\nit in the public list, considering its threat level.)\n"},{"id":"423272","messageId":"20210429145424.515208-1-matheus.bernardino@usp.br","threadId":"55588","inReplyTo":"CAM7=BFooqCo3vn+yy6uM26szZDAcbx8M=sZS1znucJvHRbtxHg@mail.gmail.com","subject":"Re: A possible divide by zero problem in read-cache.c","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-04-29T14:54:24Z","receivedAt":"2021-04-29T14:54:40Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Apr 29, 2021 at 11:33 AM Yiyuan guo <yguoaz@gmail.com> wrote:\n>\n> Hello, git developers.\n> I have found a possible divide by zero problem in read-cache.c. Here\n> is the trace (with links to code location) for triggering the bug:\n>\n> Step 0: (In function do_read_index) [ link:\n> https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2216\n> ]\n>       nr_threads = istate->cache_nr / THREAD_COST;\n> If istate->cache_nr == 0, nr_threads will also obtain 0 value.\n>\n> Step 1: (calling another function load_cache_entries_threaded with\n> nr_threads as an argument )  [ link:\n> https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2247\n> ]\n>       src_offset += load_cache_entries_threaded(istate, mmap,\n> mmap_size, nr_threads, ieot);\n\nHmm, this function call is guarded by an `if (ieot)` block:\n\n\tif (ieot) {\n\t\tsrc_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);\n\t\tfree(ieot);\n\t} else {\n\t\tsrc_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);\n\t}\n\n\nAnd `ieot` will only get a non-NULL value if this previous assignment was\nexecuted:\n\n\tif (extension_offset && nr_threads > 1)\n\t\tieot = read_ieot_extension(mmap, mmap_size, extension_offset);\n\nSo it seems to me that we only call `load_cache_entries_threaded()` when\n`nr_threads > 1`.\n\n> Step 2:  (use nr_threads as divisor, leading to possible divide by\n> zero in function load_cache_entries_threaded) [ link:\n> https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2103\n> ]\n>       ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);\n"},{"id":"423300","messageId":"YIsdhdQX03osraFD@coredump.intra.peff.net","threadId":"55588","inReplyTo":"20210429145424.515208-1-matheus.bernardino@usp.br","subject":"Re: A possible divide by zero problem in read-cache.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-29T20:56:37Z","receivedAt":"2021-04-29T20:56:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 29, 2021 at 11:54:24AM -0300, Matheus Tavares wrote:\n\n> > Step 1: (calling another function load_cache_entries_threaded with\n> > nr_threads as an argument )  [ link:\n> > https://github.com/git/git/blob/311531c9de557d25ac087c1637818bd2aad6eb3a/read-cache.c#L2247\n> > ]\n> >       src_offset += load_cache_entries_threaded(istate, mmap,\n> > mmap_size, nr_threads, ieot);\n> \n> Hmm, this function call is guarded by an `if (ieot)` block:\n> \n> \tif (ieot) {\n> \t\tsrc_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);\n> \t\tfree(ieot);\n> \t} else {\n> \t\tsrc_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);\n> \t}\n> \n> \n> And `ieot` will only get a non-NULL value if this previous assignment was\n> executed:\n> \n> \tif (extension_offset && nr_threads > 1)\n> \t\tieot = read_ieot_extension(mmap, mmap_size, extension_offset);\n> \n> So it seems to me that we only call `load_cache_entries_threaded()` when\n> `nr_threads > 1`.\n\nThanks for tracing this. I agree that this doesn't seem to be\ntriggerable along that patch for this reason.\n\nThere is another assignment to nr_threads inside the function with the\ndivision:\n\n          if (nr_threads > ieot->nr)\n                  nr_threads = ieot->nr;\n          CALLOC_ARRAY(data, nr_threads);\n  \n          offset = ieot_start = 0;\n          ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);\n\nIs it possible for ieot->nr to be 0? Almost certainly it would be a bug\nfor something to have written it, but I wonder if a malicious file could\ntrigger this.\n\nGoing back to read_ieot_extension(), I think the answer is \"no\". It\ndoes:\n\n         /* extension size - version bytes / bytes per entry */\n          nr = (extsize - sizeof(uint32_t)) / (sizeof(uint32_t) + sizeof(uint32_t));\n          if (!nr) {\n                  error(\"invalid number of IEOT entries %d\", nr);\n                  return NULL;\n          }\n          ieot = xmalloc(sizeof(struct index_entry_offset_table)\n                         + (nr * sizeof(struct index_entry_offset)));\n          ieot->nr = nr;\n\nso it will reject such an extension.\n\nSo I think all is well here.\n\n-Peff\n"}]}