core: Fix use-after-free of scollectd::impl

If the destructor for the thread_local reactor holder runs after the
destructor for the thread_local scollectd::impl, then the
scollectd registrations contained in the native_network_stack's ipv4,
when unregistering themselves, will access an invalid scollectd::impl.

This patch fixes the problem by defining scollectd::impl before
reactor_holder, ensuring they will be destroyed in reverse order.

Signed-off-by: Duarte Nunes <[email protected]>
Message-Id: <[email protected]>

core/reactor.cc

@@ -32,7 +32,7 @@
 #include "net/posix-stack.hh"
 #include "resource.hh"
 #include "print.hh"
-#include "scollectd.hh"
+#include "scollectd-impl.hh"
 #include "util/conversions.hh"
 #include "core/future-util.hh"
 #include "thread.hh"
@@ -2296,6 +2296,21 @@ smp::get_options_description()
     return opts;
 }
 
+thread_local scollectd::impl scollectd_impl;
+
+scollectd::impl & scollectd::get_impl() {
+    return scollectd_impl;
+}
+
+struct reactor_deleter {
+    void operator()(reactor* p) {
+        p->~reactor();
+        free(p);
+    }
+};
+
+thread_local std::unique_ptr<reactor, reactor_deleter> reactor_holder;
+
 std::vector<smp::thread_adaptor> smp::_threads;
 std::experimental::optional<boost::barrier> smp::_all_event_loops_done;
 std::vector<reactor*> smp::_reactors;
@@ -2347,15 +2362,6 @@ void smp::arrive_at_event_loop_end() {
 }
 
 void smp::allocate_reactor() {
-    struct reactor_deleter {
-        void operator()(reactor* p) {
-            p->~reactor();
-            free(p);
-        }
-    };
-    static thread_local std::unique_ptr<reactor, reactor_deleter>
-        reactor_holder;
-
     assert(!reactor_holder);
 
     // we cannot just write "local_engin = new reactor" since reactor's constructor

core/scollectd-impl.hh

@@ -0,0 +1,78 @@
+/*
+ * This file is open source software, licensed to you under the terms
+ * of the Apache License, Version 2.0 (the "License").  See the NOTICE file
+ * distributed with this work for additional information regarding copyright
+ * ownership.  You may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/*
+ * Copyright (C) 2016 ScyllaDB
+ */
+
+#pragma once
+
+#include "core/scollectd.hh"
+#include "core/reactor.hh"
+
+namespace scollectd {
+
+using namespace std::chrono_literals;
+using duration = std::chrono::milliseconds;
+
+static const ipv4_addr default_addr("239.192.74.66:25826");
+static const std::chrono::milliseconds default_period(1s);
+
+class impl {
+    net::udp_channel _chan;
+    timer<> _timer;
+
+    sstring _host = "localhost";
+    ipv4_addr _addr = default_addr;
+    std::chrono::milliseconds _period = default_period;
+    uint64_t _num_packets = 0;
+    uint64_t _millis = 0;
+    uint64_t _bytes = 0;
+    double _avg = 0;
+
+public:
+    typedef std::map<type_instance_id, shared_ptr<value_list> > value_list_map;
+    typedef value_list_map::value_type value_list_pair;
+
+    void add_polled(const type_instance_id & id,
+            const shared_ptr<value_list> & values);
+    void remove_polled(const type_instance_id & id);
+    // explicitly send a type_instance value list (outside polling)
+    future<> send_metric(const type_instance_id & id,
+            const value_list & values);
+    future<> send_notification(const type_instance_id & id,
+            const sstring & msg);
+    // initiates actual value polling -> send to target "loop"
+    void start(const sstring & host, const ipv4_addr & addr, const std::chrono::milliseconds period);
+    void stop();
+
+private:
+    void arm();
+    void run();
+
+public:
+    shared_ptr<value_list> get_values(const type_instance_id & id);
+    std::vector<type_instance_id> get_instance_ids();
+
+private:
+    value_list_map _values;
+    registrations _regs;
+};
+
+impl & get_impl();
+
+};