From 50642b28ca898868117bbc5c92e16b27aed1d169 Mon Sep 17 00:00:00 2001 From: Alberto Gonzalez Date: Sun, 19 Jul 2020 18:18:06 +0100 Subject: [PATCH] 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/c/fract4dc/calcargs.cpp | 8 ++++++ fract4d/c/fract4dc/calcs.cpp | 39 ++++++++---------------------- fract4d/c/fract4dc/calcs.h | 2 +- fract4d/c/fract4dc/controllers.cpp | 12 +++------ fract4d/c/fract4dc/functions.cpp | 2 +- fract4d/c/fract4dc/images.cpp | 6 ++--- fract4d/c/model/site.cpp | 31 +++++++++++------------- fract4d/c/model/site.h | 18 ++++++++------ 8 files changed, 51 insertions(+), 67 deletions(-) diff --git a/fract4d/c/fract4dc/calcargs.cpp b/fract4d/c/fract4dc/calcargs.cpp index 4296427da..18eb83470 100644 --- a/fract4d/c/fract4dc/calcargs.cpp +++ b/fract4d/c/fract4dc/calcargs.cpp @@ -1,4 +1,5 @@ #include "Python.h" +#include #include "calcargs.h" @@ -17,10 +18,13 @@ 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 lock(ref_count_mutex); Py_XINCREF(pycmap); } @@ -28,6 +32,7 @@ void calc_args::set_pfo(PyObject *pypfo_) { pypfo = pypfo_; pfo = (loaders::pf_fromcapsule(pypfo))->pfo; + const std::lock_guard lock(ref_count_mutex); Py_XINCREF(pypfo); } @@ -35,12 +40,14 @@ void calc_args::set_im(PyObject *pyim_) { pyim = pyim_; im = images::image_fromcapsule(pyim); + const std::lock_guard 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 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 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 index 2c4555061..2c8910eed 100644 --- a/fract4d/c/fract4dc/calcs.cpp +++ b/fract4d/c/fract4dc/calcs.cpp @@ -1,5 +1,6 @@ #include "Python.h" -#include +#include +#include #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 index 344f70225..cc69ea47a 100644 --- 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 index ee24a7b08..e8a8579bb 100644 --- a/fract4d/c/fract4dc/controllers.cpp +++ b/fract4d/c/fract4dc/controllers.cpp @@ -3,7 +3,7 @@ #include #include -#include +#include #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 index 9f9fc8fad..29458d77e 100644 --- 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 index fd524ec04..1785ef9f2 100644 --- 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 index 35f30c115..e7847bcc1 100644 --- a/fract4d/c/model/site.cpp +++ b/fract4d/c/model/site.cpp @@ -1,30 +1,29 @@ #include +#include #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,11 +33,10 @@ void IFractalSite::wait() inline void FDSite::send(msg_type_t type, int size, void *buf) { - pthread_mutex_lock(&write_lock); + const std::lock_guard 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) @@ -46,7 +44,6 @@ 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 index 6324f4980..ea03002e0 100644 --- 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 +#include +#include +#include #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,11 +50,11 @@ 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 @@ -61,7 +62,6 @@ 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 interrupted; + std::mutex write_lock; + + inline void send(msg_type_t type, int size, void *buf); }; #endif \ No newline at end of file