git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* 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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).