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

Message ID 20180806213527.19489-1-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] raft: Fix use-after-free error in raft_store_snapshot().
Related show

Commit Message

Ben Pfaff Aug. 6, 2018, 9:35 p.m.
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@ovn.org>
---
 ovsdb/raft.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Mark Michelson Aug. 7, 2018, 6:26 p.m. | #1
Looks good to me.

Acked-by: Mark Michelson <mmichels@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@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]);
>       }
>
Ben Pfaff Aug. 7, 2018, 7:25 p.m. | #2
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@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@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]);
> >      }
> >
>

Patch

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]);
     }