| Message ID | 20190615161135.57269-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsmonitor: avoid signed integer overflow / infinite loop | expand |
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
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
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) {
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(-)