From e284b7233fb459a7a6d4ce0f98371b34b3639d2b Mon Sep 17 00:00:00 2001 From: Rob Pilling Date: Tue, 22 Oct 2019 19:55:55 +0100 Subject: [PATCH] Perform HASHTAB_ITER bookkeeping before user-code The `HASHTAB_ITER` logic keeps track of how many entries in the hash table are left to visit, decrementing this on each iteration of the loop. This was previously decremented at the end of the loop body: ```c size_t hi##todo_ = hi##ht_->ht_used; for (hashitem_T *hi = hi##ht_->ht_array; hi##todo_; hi++) { if (!HASHITEM_EMPTY(hi)) { { } hi##todo_--; // <--- important decrement here } } ``` This meant that if the body of the loop (substituted in via macro expansion) contained a `continue` statement, we'd skip decrementing our counter, meaning we'd iterate too many times over the hash table, usually leading to an out of bounds read beyond the hash table's memory, or uninitialised/null pointers from unused hash table slots. Decrementing `hi##todo` before the arbitrary loop body protects us from this, and has no adverse side-effects since only the macro code can (or should) use this variable. Before this commit, no code within `HASHTAB_ITER()` contained a `continue`, meaning this bug was left dormant and the fix has a very minimal chance of introducing any bugs. --- src/nvim/hashtab.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/hashtab.h b/src/nvim/hashtab.h index a70a8bea63..19633d455f 100644 --- a/src/nvim/hashtab.h +++ b/src/nvim/hashtab.h @@ -81,10 +81,10 @@ typedef struct hashtable_S { size_t hi##todo_ = hi##ht_->ht_used; \ for (hashitem_T *hi = hi##ht_->ht_array; hi##todo_; hi++) { \ if (!HASHITEM_EMPTY(hi)) { \ + hi##todo_--; \ { \ code \ } \ - hi##todo_--; \ } \ } \ } while (0)