sched: free aligned_alloc() with free()

Memory allocated with aligned_alloc() should be freed with free(), not
with delete - although in our actual implemention those are really the
same thing. gcc 11 warns about using sched::thread::make() - which uses
align_alloc() - and the deleting it with "delete", using
std::unique_ptr.

Gcc only warns about this issue in sched.cc so we only fix that file,
but note that other places of the code also have unique_ptr<thread>
and should eventually use a similar fix.

The fix is a new method sched::thread::make_unique<> which returns
a std::unique_ptr that holds a thread a knows how to properly delete it.

Signed-off-by: Nadav Har'El <[email protected]>
Message-Id: <[email protected]>

Author: nyh

core/sched.cc

@@ -135,7 +135,7 @@ class thread::reaper {
 private:
     mutex _mtx;
     std::list<thread*> _zombies;
-    std::unique_ptr<thread> _thread;
+    thread_unique_ptr _thread;
 };
 
 cpu::cpu(unsigned _id)
@@ -552,7 +552,7 @@ void thread::pin(cpu *target_cpu)
     // load balancer thread) but a "good-enough" dirty solution is to
     // temporarily create a new ad-hoc thread, "wakeme".
     bool do_wakeme = false;
-    std::unique_ptr<thread> wakeme(thread::make([&] () {
+    thread_unique_ptr wakeme(thread::make_unique([&] () {
         wait_until([&] { return do_wakeme; });
         t.wake();
     }, sched::thread::attr().pin(source_cpu)));
@@ -590,7 +590,7 @@ void thread::pin(thread *t, cpu *target_cpu)
     // where the target thread is currently running. We start here a new
     // helper thread to follow the target thread's CPU. We could have also
     // re-used an existing thread (e.g., the load balancer thread).
-    std::unique_ptr<thread> helper(thread::make([&] {
+    thread_unique_ptr helper(thread::make_unique([&] {
         WITH_LOCK(irq_lock) {
             // This thread started on the same CPU as t, but by now t might
             // have moved. If that happened, we need to move too.
@@ -701,7 +701,7 @@ void thread::unpin()
         }
         return;
     }
-    std::unique_ptr<thread> helper(thread::make([this] {
+    thread_unique_ptr helper(thread::make_unique([this] {
         WITH_LOCK(preempt_lock) {
             // helper thread started on the same CPU as "this", but by now
             // "this" might migrated. If that happened helper need to migrate.
@@ -973,7 +973,7 @@ thread::thread(std::function<void ()> func, attr attr, bool main, bool app)
     , _migration_lock_counter(0)
     , _pinned(false)
     , _id(0)
-    , _cleanup([this] { delete this; })
+    , _cleanup([this] { dispose(this); })
     , _app(app)
     , _joiner(nullptr)
 {
@@ -1600,7 +1600,7 @@ bool operator<(const timer_base& t1, const timer_base& t2)
 }
 
 thread::reaper::reaper()
-    : _mtx{}, _zombies{}, _thread(thread::make([=] { reap(); }))
+    : _mtx{}, _zombies{}, _thread(thread::make_unique([=] { reap(); }))
 {
     _thread->start();
 }

include/osv/sched.hh

@@ -428,6 +428,19 @@ public:
         }
         return new(p) thread(std::forward<Args>(args)...);
     }
+    // Since make() doesn't necessarily allocate with "new", dispose() should
+    // be used to free it, not "delete". However, in practice our new and
+    // delete are the same, so delete is fine.
+    static void dispose(thread* p) {
+        p->~thread();
+        free(p);
+    }
+    using thread_unique_ptr = std::unique_ptr<thread, decltype(&thread::dispose)>;
+    template <typename... Args>
+    static thread_unique_ptr make_unique(Args&&... args) {
+        return thread_unique_ptr(make(std::forward<Args>(args)...),
+        thread::dispose);
+    }
 private:
     explicit thread(std::function<void ()> func, attr attributes = attr(),
             bool main = false, bool app = false);