diff mbox series

[ovs-dev,04/10] raft: Avoid use-after-free error in raft_update_commit_index().

Message ID 20181115060534.7146-4-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev,01/10] ovsdb-idl: Avoid sending transactions when the DB is not synced up. | expand

Commit Message

Ben Pfaff Nov. 15, 2018, 6:05 a.m. UTC 
raft_update_commit_index() iterates over a sequence of entries that may
have up to two components: a set of servers and a piece of data.  When
a set of servers is present, it calls raft_run_reconfigure(), which can
call through the following chain of functions in some cases:

   raft_log_reconfiguration()
   raft_command_execute__()
   raft_command_initiate()
   raft_write_entry()
   raft_add_entry()

and raft_add_entry() can reallocate raft->entries, which turns the pointer
'e' that raft_update_commit_index() has to the current entry into a wild
pointer.

This commit fixes the problem.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/raft.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
diff mbox series

Patch

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index 6c5c6068f1ac..b682738a92f6 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -2567,9 +2567,6 @@  raft_update_commit_index(struct raft *raft, uint64_t new_commit_index)
         while (raft->commit_index < new_commit_index) {
             uint64_t index = ++raft->commit_index;
             const struct raft_entry *e = raft_get_entry(raft, index);
-            if (e->servers) {
-                raft_run_reconfigure(raft);
-            }
             if (e->data) {
                 struct raft_command *cmd
                     = raft_find_command_by_index(raft, index);
@@ -2577,6 +2574,12 @@  raft_update_commit_index(struct raft *raft, uint64_t new_commit_index)
                     raft_command_complete(raft, cmd, RAFT_CMD_SUCCESS);
                 }
             }
+            if (e->servers) {
+                /* raft_run_reconfigure() can write a new Raft entry, which can
+                 * reallocate raft->entries, which would invalidate 'e', so
+                 * this case must be last, after the one for 'e->data'. */
+                raft_run_reconfigure(raft);
+            }
         }
     } else {
         raft->commit_index = new_commit_index;