{"thread":{"id":"51322","subject":"[PATCH] fsmonitor: avoid signed integer overflow / infinite loop","startedAt":"2019-06-15T16:12:09Z","lastAt":"2019-06-17T07:25:23Z","messageCount":3,"participants":["Carlo Marcelo Arenas Belón","Derrick Stolee","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"377303","messageId":"20190615161135.57269-1-carenas@gmail.com","threadId":"51322","inReplyTo":null,"subject":"[PATCH] fsmonitor: avoid signed integer overflow / infinite loop","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-06-15T16:11:35Z","receivedAt":"2019-06-15T16:12:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"883e248b8a (\"fsmonitor: teach git to optionally utilize a file system\nmonitor to speed up detecting new or changed files.\", 2017-09-22) uses\nan int in a loop that would wrap if index_state->cache_nr (unsigned)\nis bigger than INT_MAX\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n fsmonitor.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1dee0aded1..231e83a94d 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -56,7 +56,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \n void fill_fsmonitor_bitmap(struct index_state *istate)\n {\n-\tint i;\n+\tunsigned int i;\n \tistate->fsmonitor_dirty = ewah_new();\n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n@@ -134,7 +134,7 @@ void refresh_fsmonitor(struct index_state *istate)\n \tsize_t bol; /* beginning of line */\n \tuint64_t last_update;\n \tchar *buf;\n-\tint i;\n+\tunsigned int i;\n \n \tif (!core_fsmonitor || istate->fsmonitor_has_run_once)\n \t\treturn;\n@@ -192,7 +192,7 @@ void refresh_fsmonitor(struct index_state *istate)\n \n void add_fsmonitor(struct index_state *istate)\n {\n-\tint i;\n+\tunsigned int i;\n \n \tif (!istate->fsmonitor_last_update) {\n \t\ttrace_printf_key(&trace_fsmonitor, \"add fsmonitor\");\n@@ -225,7 +225,7 @@ void remove_fsmonitor(struct index_state *istate)\n \n void tweak_fsmonitor(struct index_state *istate)\n {\n-\tint i;\n+\tunsigned int i;\n \tint fsmonitor_enabled = git_config_get_fsmonitor();\n \n \tif (istate->fsmonitor_dirty) {\n-- \n2.22.0\n\n"},{"id":"377341","messageId":"0915f2bc-5818-3bf5-34c2-7c783363cc34@gmail.com","threadId":"51322","inReplyTo":"20190615161135.57269-1-carenas@gmail.com","subject":"Re: [PATCH] fsmonitor: avoid signed integer overflow / infinite loop","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-17T01:17:24Z","receivedAt":"2019-06-17T01:17:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/2019 12:11 PM, Carlo Marcelo Arenas Belón wrote:\n> 883e248b8a (\"fsmonitor: teach git to optionally utilize a file system\n> monitor to speed up detecting new or changed files.\", 2017-09-22) uses\n> an int in a loop that would wrap if index_state->cache_nr (unsigned)\n> is bigger than INT_MAX\n\nThanks for catching this. I wonder if there is a compiler setting or\nstatic analysis that caught this so we can avoid the issue in the future.\n\nAlso, INT_MAX is ~2.1 billion, and the largest indexes I know about have\naround 4 million entries, so it is unlikely that you hit this as a runtime\nerror. Still worth fixing!\n\nThanks,\n-Stolee\n"},{"id":"377348","messageId":"CAPUEspip4ae7W+DwP1A=DC48v64KxcgV+5EdMYHvkYCpf9UX+A@mail.gmail.com","threadId":"51322","inReplyTo":"0915f2bc-5818-3bf5-34c2-7c783363cc34@gmail.com","subject":"Re: [PATCH] fsmonitor: avoid signed integer overflow / infinite loop","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-06-17T07:25:09Z","receivedAt":"2019-06-17T07:25:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, Jun 16, 2019 at 6:17 PM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> Thanks for catching this. I wonder if there is a compiler setting or\n> static analysis that caught this so we can avoid the issue in the future.\n\n-Wsign-compare would definitely raise a warning about this, but we have\ncurrently 956 of those in master (which is why they are currently being\nsuppressed), with about 112 of those in similar code.\n\nit is important to also mention that cache_nr/active_nr is unsigned\nsince the first\nversion of git, so the issue isn't new, neither specific to fsmonitor,\nand as you\npointed out, unlikely to manifest itself.\n\nCarlo\n"}]}