fract4dc sanitize (#154)

* fix heap-use-after-free * fix data race on calc_args ref counting management ( * fix data race on site interrupted flag * fix debug prints * use std::atomic instead volatile and mutex for site interrupted flag
fract4d · Jul 19, 2020 · 50642b2 · 50642b2
1 parent 68d6da0
commit 50642b2
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 67 deletions.
diff --git a/fract4d/c/fract4dc/calcargs.cpp b/fract4d/c/fract4dc/calcargs.cpp
@@ -1,4 +1,5 @@
 #include "Python.h"
+#include <mutex>
 
 #include "calcargs.h"
 
@@ -17,30 +18,36 @@ calc_args::calc_args()
     params = new double[N_PARAMS];
 }
 
+static std::mutex ref_count_mutex;
+
 void calc_args::set_cmap(PyObject *pycmap_)
 {
     pycmap = pycmap_;
     cmap = colormaps::cmap_fromcapsule(pycmap);
+    const std::lock_guard<std::mutex> lock(ref_count_mutex);
     Py_XINCREF(pycmap);
 }
 
 void calc_args::set_pfo(PyObject *pypfo_)
 {
     pypfo = pypfo_;
     pfo = (loaders::pf_fromcapsule(pypfo))->pfo;
+    const std::lock_guard<std::mutex> lock(ref_count_mutex);
     Py_XINCREF(pypfo);
 }
 
 void calc_args::set_im(PyObject *pyim_)
 {
     pyim = pyim_;
     im = images::image_fromcapsule(pyim);
+    const std::lock_guard<std::mutex> lock(ref_count_mutex);
     Py_XINCREF(pyim);
 }
 void calc_args::set_site(PyObject *pysite_)
 {
     pysite = pysite_;
     site = sites::site_fromcapsule(pysite);
+    const std::lock_guard<std::mutex> lock(ref_count_mutex);
     Py_XINCREF(pysite);
 }
 
@@ -50,6 +57,7 @@ calc_args::~calc_args()
 #ifdef DEBUG_CREATION
     fprintf(stderr, "%p : CA : DTOR\n", this);
 #endif
+    const std::lock_guard<std::mutex> lock(ref_count_mutex);
     Py_XDECREF(pycmap);
     Py_XDECREF(pypfo);
     Py_XDECREF(pyim);

diff --git a/fract4d/c/fract4dc/calcs.cpp b/fract4d/c/fract4dc/calcs.cpp
@@ -1,5 +1,6 @@
 #include "Python.h"
-#include <pthread.h>
+#include <thread>
+#include <iostream>
 
 #include "calcs.h"
 
@@ -45,26 +46,11 @@ namespace calcs {
 
         if (cargs->asynchronous)
         {
-            cargs->site->interrupt();
-            cargs->site->wait();
-
-            // cargs->site->start(cargs);
-            cargs->site->start();
-
-            pthread_t tid;
-
-            /* create low-priority attribute block */
-            pthread_attr_t lowprio_attr;
-            //struct sched_param lowprio_param;
-            pthread_attr_init(&lowprio_attr);
-            //lowprio_param.sched_priority = sched_get_priority_min(SCHED_OTHER);
-            //pthread_attr_setschedparam(&lowprio_attr, &lowprio_param);
-
-            /* start the calculation thread */
-            pthread_create(&tid, &lowprio_attr, calculation_thread, (void *)cargs);
-            assert(tid != 0);
-
-            cargs->site->set_tid(tid);
+            auto &site = *cargs->site;
+            site.interrupt();
+            site.wait();
+            site.start();
+            site.set_thread(std::thread(calculation_thread, cargs));
         }
         else
         {
@@ -91,14 +77,11 @@ namespace calcs {
 }
 
 
-void * calculation_thread(void *vdata)
+void * calculation_thread(calc_args *args)
 {
-    calc_args *args = (calc_args *)vdata;
-
 #ifdef DEBUG_THREADS
-    fprintf(stderr, "%p : CA : CALC(%d)\n", args, pthread_self());
+    std::cerr << args << " : CA : CALC(" << std::this_thread::get_id() << ")\n";
 #endif
-
     calc(
         args->options,
         args->params,
@@ -108,11 +91,9 @@ void * calculation_thread(void *vdata)
         args->im,
         0 // debug_flags
     );
-
 #ifdef DEBUG_THREADS
-    fprintf(stderr, "%p : CA : ENDCALC(%d)\n", args, pthread_self());
+    std::cerr << args << " : CA : ENDCALC(" << std::this_thread::get_id() << ")\n";
 #endif
-
     delete args;
     return NULL;
 }

diff --git a/fract4d/c/fract4dc/calcs.h b/fract4d/c/fract4dc/calcs.h
@@ -6,7 +6,7 @@ typedef struct _object PyObject;
 struct calc_args;
 
 calc_args * parse_calc_args(PyObject *args, PyObject *kwds);
-void * calculation_thread(void *vdata);
+void * calculation_thread(calc_args *args);
 
 namespace calcs {
     PyObject * pystop_calc(PyObject *self, PyObject *args);

diff --git a/fract4d/c/fract4dc/controllers.cpp b/fract4d/c/fract4dc/controllers.cpp
@@ -3,7 +3,7 @@
 
 #include <dlfcn.h>
 #include <cassert>
-#include <pthread.h>
+#include <thread>
 
 #include "pf.h"
 
@@ -70,8 +70,7 @@ void fractal_controller::start_calculating(
     image = images::image_fromcapsule(py_image);
     Py_XINCREF(py_image);
 
-    auto calc_fn = [](void *data) mutable -> void* {
-        fractal_controller *fc = (fractal_controller *)data;
+    auto calc_fn = [](fractal_controller *fc) mutable -> void* {
         calc(
             fc->c_options,
             fc->c_pos_params,
@@ -88,13 +87,10 @@ void fractal_controller::start_calculating(
         site->interrupt();
         site->wait();
         site->start();
-        pthread_t tid;
-        pthread_create(&tid, nullptr, calc_fn, (void *)this);
-        assert(tid != 0);
-        site->set_tid(tid);
+        site->set_thread(std::thread(calc_fn, this));
     } else {
         Py_BEGIN_ALLOW_THREADS
-        calc_fn((void *)this);
+        calc_fn(this);
         Py_END_ALLOW_THREADS
     }
 }

diff --git a/fract4d/c/fract4dc/functions.cpp b/fract4d/c/fract4dc/functions.cpp
@@ -170,7 +170,7 @@ void pyff_delete(PyObject *pyff)
 {
     ffHandle *ff = (ffHandle *)PyCapsule_GetPointer(pyff, OBTYPE_FFH);
 #ifdef DEBUG_CREATION
-    fprintf(stderr, "%p : FF : DTOR\n", ffh);
+    fprintf(stderr, "%p : FF : DTOR\n", ff);
 #endif
     delete ff->ff;
     Py_DECREF(ff->pyhandle);

diff --git a/fract4d/c/fract4dc/images.cpp b/fract4d/c/fract4dc/images.cpp
@@ -33,7 +33,7 @@ namespace images {
         }
 
     #ifdef DEBUG_CREATION
-        fprintf(stderr, "%p : IM : CTOR\n", i);
+        fprintf(stderr, "%p : IM : CTOR %dx%d \n", i, x, y);
     #endif
 
         PyObject *pyret = PyCapsule_New(i, OBTYPE_IMAGE, pyimage_delete);
@@ -462,8 +462,8 @@ namespace images {
 void pyimage_delete(PyObject *pyimage)
 {
     IImage *im = images::image_fromcapsule(pyimage);
-    #ifdef DEBUG_CREATION
-    fprintf(stderr, "%p : IM : DTOR\n", image);
+#ifdef DEBUG_CREATION
+    fprintf(stderr, "%p : IM : DTOR\n", im);
 #endif
     delete im;
 }

diff --git a/fract4d/c/model/site.cpp b/fract4d/c/model/site.cpp
@@ -1,30 +1,29 @@
 #include <unistd.h>
+#include <iostream>
 
 #include "site.h"
-
 #include "model/stats.h"
 
-
-IFractalSite::IFractalSite() {
-    tid = (pthread_t)0;
+IFractalSite::~IFractalSite() {
+    wait();
 }
 
-void IFractalSite::set_tid(pthread_t tid_)
+void IFractalSite::set_thread(std::thread t)
 {
 #ifdef DEBUG_THREADS
-    fprintf(stderr, "%p : CA : SET(%p)\n", this, tid_);
+    std::cerr << this << " : CA : SET(" << t.get_id() << ")\n";
 #endif
-    tid = tid_;
+    m_thread = std::move(t);
 }
 
 void IFractalSite::wait()
 {
-    if (tid != 0)
+    if (m_thread.joinable())
     {
 #ifdef DEBUG_THREADS
-        fprintf(stderr, "%p : CA : WAIT(%p)\n", this, tid);
+        std::cerr << this << " : CA : WAIT(" << m_thread.get_id() << ")\n";
 #endif
-        pthread_join(tid, NULL);
+        m_thread.join();
     }
 }
 
@@ -34,19 +33,17 @@ void IFractalSite::wait()
 
 inline void FDSite::send(msg_type_t type, int size, void *buf)
 {
-    pthread_mutex_lock(&write_lock);
+    const std::lock_guard<std::mutex> lock(write_lock);
     write(fd, &type, sizeof(type));
     write(fd, &size, sizeof(size));
     write(fd, buf, size);
-    pthread_mutex_unlock(&write_lock);
 }
 
 FDSite::FDSite(int fd_) : fd(fd_), interrupted(false)
 {
 #ifdef DEBUG_CREATION
     fprintf(stderr, "%p : FD : CTOR\n", this);
 #endif
-    pthread_mutex_init(&write_lock, NULL);
 }
 
 void FDSite::iters_changed(int numiters)
@@ -62,7 +59,7 @@ void FDSite::tolerance_changed(double tolerance)
 // we've drawn a rectangle of image
 void FDSite::image_changed(int x1, int y1, int x2, int y2)
 {
-    if (!interrupted)
+    if (!is_interrupted())
     {
         int buf[4] = {x1, y1, x2, y2};
         send(IMAGE, sizeof(buf), &buf[0]);
@@ -71,7 +68,7 @@ void FDSite::image_changed(int x1, int y1, int x2, int y2)
 // estimate of how far through current pass we are
 void FDSite::progress_changed(float progress)
 {
-    if (!interrupted)
+    if (!is_interrupted())
     {
         int percentdone = (int)(100.0 * progress);
         send(PROGRESS, sizeof(percentdone), &percentdone);
@@ -80,7 +77,7 @@ void FDSite::progress_changed(float progress)
 
 void FDSite::stats_changed(pixel_stat_t &stats)
 {
-    if (!interrupted)
+    if (!is_interrupted())
     {
         send(STATS, sizeof(stats), &stats);
     }
@@ -118,7 +115,7 @@ void FDSite::pixel_changed(
 void FDSite::interrupt()
 {
 #ifdef DEBUG_THREADS
-    fprintf(stderr, "%p : CA : INT(%p)\n", this, tid);
+    std::cerr << this << " : CA : INT(" << m_thread.get_id() << ")\n";
 #endif
     interrupted = true;
 }

diff --git a/fract4d/c/model/site.h b/fract4d/c/model/site.h
@@ -1,7 +1,9 @@
 #ifndef __SITE_H_INCLUDED__
 #define __SITE_H_INCLUDED__
 
-#include <pthread.h>
+#include <thread>
+#include <mutex>
+#include <atomic>
 
 #include "model/enums.h"
 
@@ -20,8 +22,7 @@ typedef struct s_pixel_stat pixel_stat_t;
 class IFractalSite
 {
 public:
-    IFractalSite();
-    virtual ~IFractalSite(){};
+    virtual ~IFractalSite();
     // the parameters have changed (usually due to auto-deepening)
     virtual void iters_changed(int numiters) = 0;
     // tolerance has changed due to auto-tolerance
@@ -49,19 +50,18 @@ class IFractalSite
     virtual void interrupt() = 0;
     virtual void start() = 0;
     // having started it, set the thread id of the calc thread to wait for
-    virtual void set_tid(pthread_t tid);
+    virtual void set_thread(std::thread t);
     // wait for it to finish
     virtual void wait();
 protected:
-    pthread_t tid;
+    std::thread m_thread;
 };
 
 // write the callbacks to a file descriptor
 class FDSite : public IFractalSite
 {
 public:
     FDSite(int fd_);
-    inline void send(msg_type_t type, int size, void *buf);
     void iters_changed(int numiters);
     void tolerance_changed(double tolerance);
     void image_changed(int x1, int y1, int x2, int y2);
@@ -81,8 +81,10 @@ class FDSite : public IFractalSite
     ~FDSite();
 private:
     int fd;
-    volatile bool interrupted;
-    pthread_mutex_t write_lock;
+    std::atomic<bool> interrupted;
+    std::mutex write_lock;
+
+    inline void send(msg_type_t type, int size, void *buf);
 };
 
 #endif