[PATCH] fsmonitor: avoid signed integer overflow / infinite loop

[PATCH] fsmonitor: avoid signed integer overflow / infinite loop

[Carlo Marcelo Arenas Belón]

883e248b8a ("fsmonitor: teach git to optionally utilize a file system
monitor to speed up detecting new or changed files.", 2017-09-22) uses
an int in a loop that would wrap if index_state->cache_nr (unsigned)
is bigger than INT_MAX

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 fsmonitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 1dee0aded1..231e83a94d 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -56,7 +56,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 
 void fill_fsmonitor_bitmap(struct index_state *istate)
 {
-	int i;
+	unsigned int i;
 	istate->fsmonitor_dirty = ewah_new();
 	for (i = 0; i < istate->cache_nr; i++)
 		if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
@@ -134,7 +134,7 @@ void refresh_fsmonitor(struct index_state *istate)
 	size_t bol; /* beginning of line */
 	uint64_t last_update;
 	char *buf;
-	int i;
+	unsigned int i;
 
 	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
 		return;
@@ -192,7 +192,7 @@ void refresh_fsmonitor(struct index_state *istate)
 
 void add_fsmonitor(struct index_state *istate)
 {
-	int i;
+	unsigned int i;
 
 	if (!istate->fsmonitor_last_update) {
 		trace_printf_key(&trace_fsmonitor, "add fsmonitor");
@@ -225,7 +225,7 @@ void remove_fsmonitor(struct index_state *istate)
 
 void tweak_fsmonitor(struct index_state *istate)
 {
-	int i;
+	unsigned int i;
 	int fsmonitor_enabled = git_config_get_fsmonitor();
 
 	if (istate->fsmonitor_dirty) {
-- 
2.22.0

Re: [PATCH] fsmonitor: avoid signed integer overflow / infinite loop

[Derrick Stolee]

On 6/15/2019 12:11 PM, Carlo Marcelo Arenas Belón wrote:
> 883e248b8a ("fsmonitor: teach git to optionally utilize a file system
> monitor to speed up detecting new or changed files.", 2017-09-22) uses
> an int in a loop that would wrap if index_state->cache_nr (unsigned)
> is bigger than INT_MAX

Thanks for catching this. I wonder if there is a compiler setting or
static analysis that caught this so we can avoid the issue in the future.

Also, INT_MAX is ~2.1 billion, and the largest indexes I know about have
around 4 million entries, so it is unlikely that you hit this as a runtime
error. Still worth fixing!

Thanks,
-Stolee

Re: [PATCH] fsmonitor: avoid signed integer overflow / infinite loop

[Carlo Arenas]

On Sun, Jun 16, 2019 at 6:17 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> Thanks for catching this. I wonder if there is a compiler setting or
> static analysis that caught this so we can avoid the issue in the future.

-Wsign-compare would definitely raise a warning about this, but we have
currently 956 of those in master (which is why they are currently being
suppressed), with about 112 of those in similar code.

it is important to also mention that cache_nr/active_nr is unsigned
since the first
version of git, so the issue isn't new, neither specific to fsmonitor,
and as you
pointed out, unlikely to manifest itself.

Carlo