[ovs-dev] [PATCH] raft: Fix use-after-free error in raft_store_snapshot().

Ben Pfaff blp at ovn.org
Tue Aug 7 19:25:48 UTC 2018


Thanks, applied to master, branch-2.10, branch-2.9.

On Tue, Aug 07, 2018 at 02:26:11PM -0400, Mark Michelson wrote:
> Looks good to me.
> 
> Acked-by: Mark Michelson <mmichels at redhat.com>
> 
> On 08/06/2018 05:35 PM, Ben Pfaff wrote:
> >raft_store_snapshot() constructs a new snapshot in a local variable then
> >destroys the current snapshot and replaces it by the new one.  Until now,
> >it has not cloned the data in the new snapshot until it did the
> >replacement.  This led to the unexpected consequence that, if 'servers' in
> >the old and new snapshots was the same, then it would first be freed and
> >later cloned, which could cause a segfault.
> >
> >Multiple people reported the crash.  Gurucharan Shetty provided a
> >reproduction case.
> >
> >Signed-off-by: Ben Pfaff <blp at ovn.org>
> >---
> >  ovsdb/raft.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> >diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> >index c0c1e98977b9..02ba763e5fc4 100644
> >--- a/ovsdb/raft.c
> >+++ b/ovsdb/raft.c
> >@@ -3838,22 +3838,22 @@ raft_store_snapshot(struct raft *raft, const struct json *new_snapshot_data)
> >      }
> >      uint64_t new_log_start = raft->last_applied + 1;
> >-    const struct raft_entry new_snapshot = {
> >+    struct raft_entry new_snapshot = {
> >          .term = raft_get_term(raft, new_log_start - 1),
> >-        .data = CONST_CAST(struct json *, new_snapshot_data),
> >+        .data = json_clone(new_snapshot_data),
> >          .eid = *raft_get_eid(raft, new_log_start - 1),
> >-        .servers = CONST_CAST(struct json *,
> >-                              raft_servers_for_index(raft, new_log_start - 1)),
> >+        .servers = json_clone(raft_servers_for_index(raft, new_log_start - 1)),
> >      };
> >      struct ovsdb_error *error = raft_save_snapshot(raft, new_log_start,
> >                                                     &new_snapshot);
> >      if (error) {
> >+        raft_entry_uninit(&new_snapshot);
> >          return error;
> >      }
> >      raft->log_synced = raft->log_end - 1;
> >      raft_entry_uninit(&raft->snap);
> >-    raft_entry_clone(&raft->snap, &new_snapshot);
> >+    raft->snap = new_snapshot;
> >      for (size_t i = 0; i < new_log_start - raft->log_start; i++) {
> >          raft_entry_uninit(&raft->entries[i]);
> >      }
> >
> 


More information about the dev mailing list