1 |
From 50642b28ca898868117bbc5c92e16b27aed1d169 Mon Sep 17 00:00:00 2001 |
2 |
From: Alberto Gonzalez <mindhells@gmail.com> |
3 |
Date: Sun, 19 Jul 2020 18:18:06 +0100 |
4 |
Subject: [PATCH] fract4dc sanitize (#154) |
5 |
|
6 |
* fix heap-use-after-free |
7 |
|
8 |
* fix data race on calc_args ref counting management ( |
9 |
|
10 |
* fix data race on site interrupted flag |
11 |
|
12 |
* fix debug prints |
13 |
|
14 |
* use std::atomic instead volatile and mutex for site interrupted flag |
15 |
--- |
16 |
fract4d/c/fract4dc/calcargs.cpp | 8 ++++++ |
17 |
fract4d/c/fract4dc/calcs.cpp | 39 ++++++++---------------------- |
18 |
fract4d/c/fract4dc/calcs.h | 2 +- |
19 |
fract4d/c/fract4dc/controllers.cpp | 12 +++------ |
20 |
fract4d/c/fract4dc/functions.cpp | 2 +- |
21 |
fract4d/c/fract4dc/images.cpp | 6 ++--- |
22 |
fract4d/c/model/site.cpp | 31 +++++++++++------------- |
23 |
fract4d/c/model/site.h | 18 ++++++++------ |
24 |
8 files changed, 51 insertions(+), 67 deletions(-) |
25 |
|
26 |
diff --git a/fract4d/c/fract4dc/calcargs.cpp b/fract4d/c/fract4dc/calcargs.cpp |
27 |
index 4296427da..18eb83470 100644 |
28 |
--- a/fract4d/c/fract4dc/calcargs.cpp |
29 |
+++ b/fract4d/c/fract4dc/calcargs.cpp |
30 |
@@ -1,4 +1,5 @@ |
31 |
#include "Python.h" |
32 |
+#include <mutex> |
33 |
|
34 |
#include "calcargs.h" |
35 |
|
36 |
@@ -17,10 +18,13 @@ calc_args::calc_args() |
37 |
params = new double[N_PARAMS]; |
38 |
} |
39 |
|
40 |
+static std::mutex ref_count_mutex; |
41 |
+ |
42 |
void calc_args::set_cmap(PyObject *pycmap_) |
43 |
{ |
44 |
pycmap = pycmap_; |
45 |
cmap = colormaps::cmap_fromcapsule(pycmap); |
46 |
+ const std::lock_guard<std::mutex> lock(ref_count_mutex); |
47 |
Py_XINCREF(pycmap); |
48 |
} |
49 |
|
50 |
@@ -28,6 +32,7 @@ void calc_args::set_pfo(PyObject *pypfo_) |
51 |
{ |
52 |
pypfo = pypfo_; |
53 |
pfo = (loaders::pf_fromcapsule(pypfo))->pfo; |
54 |
+ const std::lock_guard<std::mutex> lock(ref_count_mutex); |
55 |
Py_XINCREF(pypfo); |
56 |
} |
57 |
|
58 |
@@ -35,12 +40,14 @@ void calc_args::set_im(PyObject *pyim_) |
59 |
{ |
60 |
pyim = pyim_; |
61 |
im = images::image_fromcapsule(pyim); |
62 |
+ const std::lock_guard<std::mutex> lock(ref_count_mutex); |
63 |
Py_XINCREF(pyim); |
64 |
} |
65 |
void calc_args::set_site(PyObject *pysite_) |
66 |
{ |
67 |
pysite = pysite_; |
68 |
site = sites::site_fromcapsule(pysite); |
69 |
+ const std::lock_guard<std::mutex> lock(ref_count_mutex); |
70 |
Py_XINCREF(pysite); |
71 |
} |
72 |
|
73 |
@@ -50,6 +57,7 @@ calc_args::~calc_args() |
74 |
#ifdef DEBUG_CREATION |
75 |
fprintf(stderr, "%p : CA : DTOR\n", this); |
76 |
#endif |
77 |
+ const std::lock_guard<std::mutex> lock(ref_count_mutex); |
78 |
Py_XDECREF(pycmap); |
79 |
Py_XDECREF(pypfo); |
80 |
Py_XDECREF(pyim); |
81 |
diff --git a/fract4d/c/fract4dc/calcs.cpp b/fract4d/c/fract4dc/calcs.cpp |
82 |
index 2c4555061..2c8910eed 100644 |
83 |
--- a/fract4d/c/fract4dc/calcs.cpp |
84 |
+++ b/fract4d/c/fract4dc/calcs.cpp |
85 |
@@ -1,5 +1,6 @@ |
86 |
#include "Python.h" |
87 |
-#include <pthread.h> |
88 |
+#include <thread> |
89 |
+#include <iostream> |
90 |
|
91 |
#include "calcs.h" |
92 |
|
93 |
@@ -45,26 +46,11 @@ namespace calcs { |
94 |
|
95 |
if (cargs->asynchronous) |
96 |
{ |
97 |
- cargs->site->interrupt(); |
98 |
- cargs->site->wait(); |
99 |
- |
100 |
- // cargs->site->start(cargs); |
101 |
- cargs->site->start(); |
102 |
- |
103 |
- pthread_t tid; |
104 |
- |
105 |
- /* create low-priority attribute block */ |
106 |
- pthread_attr_t lowprio_attr; |
107 |
- //struct sched_param lowprio_param; |
108 |
- pthread_attr_init(&lowprio_attr); |
109 |
- //lowprio_param.sched_priority = sched_get_priority_min(SCHED_OTHER); |
110 |
- //pthread_attr_setschedparam(&lowprio_attr, &lowprio_param); |
111 |
- |
112 |
- /* start the calculation thread */ |
113 |
- pthread_create(&tid, &lowprio_attr, calculation_thread, (void *)cargs); |
114 |
- assert(tid != 0); |
115 |
- |
116 |
- cargs->site->set_tid(tid); |
117 |
+ auto &site = *cargs->site; |
118 |
+ site.interrupt(); |
119 |
+ site.wait(); |
120 |
+ site.start(); |
121 |
+ site.set_thread(std::thread(calculation_thread, cargs)); |
122 |
} |
123 |
else |
124 |
{ |
125 |
@@ -91,14 +77,11 @@ namespace calcs { |
126 |
} |
127 |
|
128 |
|
129 |
-void * calculation_thread(void *vdata) |
130 |
+void * calculation_thread(calc_args *args) |
131 |
{ |
132 |
- calc_args *args = (calc_args *)vdata; |
133 |
- |
134 |
#ifdef DEBUG_THREADS |
135 |
- fprintf(stderr, "%p : CA : CALC(%d)\n", args, pthread_self()); |
136 |
+ std::cerr << args << " : CA : CALC(" << std::this_thread::get_id() << ")\n"; |
137 |
#endif |
138 |
- |
139 |
calc( |
140 |
args->options, |
141 |
args->params, |
142 |
@@ -108,11 +91,9 @@ void * calculation_thread(void *vdata) |
143 |
args->im, |
144 |
0 // debug_flags |
145 |
); |
146 |
- |
147 |
#ifdef DEBUG_THREADS |
148 |
- fprintf(stderr, "%p : CA : ENDCALC(%d)\n", args, pthread_self()); |
149 |
+ std::cerr << args << " : CA : ENDCALC(" << std::this_thread::get_id() << ")\n"; |
150 |
#endif |
151 |
- |
152 |
delete args; |
153 |
return NULL; |
154 |
} |
155 |
diff --git a/fract4d/c/fract4dc/calcs.h b/fract4d/c/fract4dc/calcs.h |
156 |
index 344f70225..cc69ea47a 100644 |
157 |
--- a/fract4d/c/fract4dc/calcs.h |
158 |
+++ b/fract4d/c/fract4dc/calcs.h |
159 |
@@ -6,7 +6,7 @@ typedef struct _object PyObject; |
160 |
struct calc_args; |
161 |
|
162 |
calc_args * parse_calc_args(PyObject *args, PyObject *kwds); |
163 |
-void * calculation_thread(void *vdata); |
164 |
+void * calculation_thread(calc_args *args); |
165 |
|
166 |
namespace calcs { |
167 |
PyObject * pystop_calc(PyObject *self, PyObject *args); |
168 |
diff --git a/fract4d/c/fract4dc/controllers.cpp b/fract4d/c/fract4dc/controllers.cpp |
169 |
index ee24a7b08..e8a8579bb 100644 |
170 |
--- a/fract4d/c/fract4dc/controllers.cpp |
171 |
+++ b/fract4d/c/fract4dc/controllers.cpp |
172 |
@@ -3,7 +3,7 @@ |
173 |
|
174 |
#include <dlfcn.h> |
175 |
#include <cassert> |
176 |
-#include <pthread.h> |
177 |
+#include <thread> |
178 |
|
179 |
#include "pf.h" |
180 |
|
181 |
@@ -70,8 +70,7 @@ void fractal_controller::start_calculating( |
182 |
image = images::image_fromcapsule(py_image); |
183 |
Py_XINCREF(py_image); |
184 |
|
185 |
- auto calc_fn = [](void *data) mutable -> void* { |
186 |
- fractal_controller *fc = (fractal_controller *)data; |
187 |
+ auto calc_fn = [](fractal_controller *fc) mutable -> void* { |
188 |
calc( |
189 |
fc->c_options, |
190 |
fc->c_pos_params, |
191 |
@@ -88,13 +87,10 @@ void fractal_controller::start_calculating( |
192 |
site->interrupt(); |
193 |
site->wait(); |
194 |
site->start(); |
195 |
- pthread_t tid; |
196 |
- pthread_create(&tid, nullptr, calc_fn, (void *)this); |
197 |
- assert(tid != 0); |
198 |
- site->set_tid(tid); |
199 |
+ site->set_thread(std::thread(calc_fn, this)); |
200 |
} else { |
201 |
Py_BEGIN_ALLOW_THREADS |
202 |
- calc_fn((void *)this); |
203 |
+ calc_fn(this); |
204 |
Py_END_ALLOW_THREADS |
205 |
} |
206 |
} |
207 |
diff --git a/fract4d/c/fract4dc/functions.cpp b/fract4d/c/fract4dc/functions.cpp |
208 |
index 9f9fc8fad..29458d77e 100644 |
209 |
--- a/fract4d/c/fract4dc/functions.cpp |
210 |
+++ b/fract4d/c/fract4dc/functions.cpp |
211 |
@@ -170,7 +170,7 @@ void pyff_delete(PyObject *pyff) |
212 |
{ |
213 |
ffHandle *ff = (ffHandle *)PyCapsule_GetPointer(pyff, OBTYPE_FFH); |
214 |
#ifdef DEBUG_CREATION |
215 |
- fprintf(stderr, "%p : FF : DTOR\n", ffh); |
216 |
+ fprintf(stderr, "%p : FF : DTOR\n", ff); |
217 |
#endif |
218 |
delete ff->ff; |
219 |
Py_DECREF(ff->pyhandle); |
220 |
diff --git a/fract4d/c/fract4dc/images.cpp b/fract4d/c/fract4dc/images.cpp |
221 |
index fd524ec04..1785ef9f2 100644 |
222 |
--- a/fract4d/c/fract4dc/images.cpp |
223 |
+++ b/fract4d/c/fract4dc/images.cpp |
224 |
@@ -33,7 +33,7 @@ namespace images { |
225 |
} |
226 |
|
227 |
#ifdef DEBUG_CREATION |
228 |
- fprintf(stderr, "%p : IM : CTOR\n", i); |
229 |
+ fprintf(stderr, "%p : IM : CTOR %dx%d \n", i, x, y); |
230 |
#endif |
231 |
|
232 |
PyObject *pyret = PyCapsule_New(i, OBTYPE_IMAGE, pyimage_delete); |
233 |
@@ -462,8 +462,8 @@ namespace images { |
234 |
void pyimage_delete(PyObject *pyimage) |
235 |
{ |
236 |
IImage *im = images::image_fromcapsule(pyimage); |
237 |
- #ifdef DEBUG_CREATION |
238 |
- fprintf(stderr, "%p : IM : DTOR\n", image); |
239 |
+#ifdef DEBUG_CREATION |
240 |
+ fprintf(stderr, "%p : IM : DTOR\n", im); |
241 |
#endif |
242 |
delete im; |
243 |
} |
244 |
diff --git a/fract4d/c/model/site.cpp b/fract4d/c/model/site.cpp |
245 |
index 35f30c115..e7847bcc1 100644 |
246 |
--- a/fract4d/c/model/site.cpp |
247 |
+++ b/fract4d/c/model/site.cpp |
248 |
@@ -1,30 +1,29 @@ |
249 |
#include <unistd.h> |
250 |
+#include <iostream> |
251 |
|
252 |
#include "site.h" |
253 |
- |
254 |
#include "model/stats.h" |
255 |
|
256 |
- |
257 |
-IFractalSite::IFractalSite() { |
258 |
- tid = (pthread_t)0; |
259 |
+IFractalSite::~IFractalSite() { |
260 |
+ wait(); |
261 |
} |
262 |
|
263 |
-void IFractalSite::set_tid(pthread_t tid_) |
264 |
+void IFractalSite::set_thread(std::thread t) |
265 |
{ |
266 |
#ifdef DEBUG_THREADS |
267 |
- fprintf(stderr, "%p : CA : SET(%p)\n", this, tid_); |
268 |
+ std::cerr << this << " : CA : SET(" << t.get_id() << ")\n"; |
269 |
#endif |
270 |
- tid = tid_; |
271 |
+ m_thread = std::move(t); |
272 |
} |
273 |
|
274 |
void IFractalSite::wait() |
275 |
{ |
276 |
- if (tid != 0) |
277 |
+ if (m_thread.joinable()) |
278 |
{ |
279 |
#ifdef DEBUG_THREADS |
280 |
- fprintf(stderr, "%p : CA : WAIT(%p)\n", this, tid); |
281 |
+ std::cerr << this << " : CA : WAIT(" << m_thread.get_id() << ")\n"; |
282 |
#endif |
283 |
- pthread_join(tid, NULL); |
284 |
+ m_thread.join(); |
285 |
} |
286 |
} |
287 |
|
288 |
@@ -34,11 +33,10 @@ void IFractalSite::wait() |
289 |
|
290 |
inline void FDSite::send(msg_type_t type, int size, void *buf) |
291 |
{ |
292 |
- pthread_mutex_lock(&write_lock); |
293 |
+ const std::lock_guard<std::mutex> lock(write_lock); |
294 |
write(fd, &type, sizeof(type)); |
295 |
write(fd, &size, sizeof(size)); |
296 |
write(fd, buf, size); |
297 |
- pthread_mutex_unlock(&write_lock); |
298 |
} |
299 |
|
300 |
FDSite::FDSite(int fd_) : fd(fd_), interrupted(false) |
301 |
@@ -46,7 +44,6 @@ FDSite::FDSite(int fd_) : fd(fd_), interrupted(false) |
302 |
#ifdef DEBUG_CREATION |
303 |
fprintf(stderr, "%p : FD : CTOR\n", this); |
304 |
#endif |
305 |
- pthread_mutex_init(&write_lock, NULL); |
306 |
} |
307 |
|
308 |
void FDSite::iters_changed(int numiters) |
309 |
@@ -62,7 +59,7 @@ void FDSite::tolerance_changed(double tolerance) |
310 |
// we've drawn a rectangle of image |
311 |
void FDSite::image_changed(int x1, int y1, int x2, int y2) |
312 |
{ |
313 |
- if (!interrupted) |
314 |
+ if (!is_interrupted()) |
315 |
{ |
316 |
int buf[4] = {x1, y1, x2, y2}; |
317 |
send(IMAGE, sizeof(buf), &buf[0]); |
318 |
@@ -71,7 +68,7 @@ void FDSite::image_changed(int x1, int y1, int x2, int y2) |
319 |
// estimate of how far through current pass we are |
320 |
void FDSite::progress_changed(float progress) |
321 |
{ |
322 |
- if (!interrupted) |
323 |
+ if (!is_interrupted()) |
324 |
{ |
325 |
int percentdone = (int)(100.0 * progress); |
326 |
send(PROGRESS, sizeof(percentdone), &percentdone); |
327 |
@@ -80,7 +77,7 @@ void FDSite::progress_changed(float progress) |
328 |
|
329 |
void FDSite::stats_changed(pixel_stat_t &stats) |
330 |
{ |
331 |
- if (!interrupted) |
332 |
+ if (!is_interrupted()) |
333 |
{ |
334 |
send(STATS, sizeof(stats), &stats); |
335 |
} |
336 |
@@ -118,7 +115,7 @@ void FDSite::pixel_changed( |
337 |
void FDSite::interrupt() |
338 |
{ |
339 |
#ifdef DEBUG_THREADS |
340 |
- fprintf(stderr, "%p : CA : INT(%p)\n", this, tid); |
341 |
+ std::cerr << this << " : CA : INT(" << m_thread.get_id() << ")\n"; |
342 |
#endif |
343 |
interrupted = true; |
344 |
} |
345 |
diff --git a/fract4d/c/model/site.h b/fract4d/c/model/site.h |
346 |
index 6324f4980..ea03002e0 100644 |
347 |
--- a/fract4d/c/model/site.h |
348 |
+++ b/fract4d/c/model/site.h |
349 |
@@ -1,7 +1,9 @@ |
350 |
#ifndef __SITE_H_INCLUDED__ |
351 |
#define __SITE_H_INCLUDED__ |
352 |
|
353 |
-#include <pthread.h> |
354 |
+#include <thread> |
355 |
+#include <mutex> |
356 |
+#include <atomic> |
357 |
|
358 |
#include "model/enums.h" |
359 |
|
360 |
@@ -20,8 +22,7 @@ typedef struct s_pixel_stat pixel_stat_t; |
361 |
class IFractalSite |
362 |
{ |
363 |
public: |
364 |
- IFractalSite(); |
365 |
- virtual ~IFractalSite(){}; |
366 |
+ virtual ~IFractalSite(); |
367 |
// the parameters have changed (usually due to auto-deepening) |
368 |
virtual void iters_changed(int numiters) = 0; |
369 |
// tolerance has changed due to auto-tolerance |
370 |
@@ -49,11 +50,11 @@ class IFractalSite |
371 |
virtual void interrupt() = 0; |
372 |
virtual void start() = 0; |
373 |
// having started it, set the thread id of the calc thread to wait for |
374 |
- virtual void set_tid(pthread_t tid); |
375 |
+ virtual void set_thread(std::thread t); |
376 |
// wait for it to finish |
377 |
virtual void wait(); |
378 |
protected: |
379 |
- pthread_t tid; |
380 |
+ std::thread m_thread; |
381 |
}; |
382 |
|
383 |
// write the callbacks to a file descriptor |
384 |
@@ -61,7 +62,6 @@ class FDSite : public IFractalSite |
385 |
{ |
386 |
public: |
387 |
FDSite(int fd_); |
388 |
- inline void send(msg_type_t type, int size, void *buf); |
389 |
void iters_changed(int numiters); |
390 |
void tolerance_changed(double tolerance); |
391 |
void image_changed(int x1, int y1, int x2, int y2); |
392 |
@@ -81,8 +81,10 @@ class FDSite : public IFractalSite |
393 |
~FDSite(); |
394 |
private: |
395 |
int fd; |
396 |
- volatile bool interrupted; |
397 |
- pthread_mutex_t write_lock; |
398 |
+ std::atomic<bool> interrupted; |
399 |
+ std::mutex write_lock; |
400 |
+ |
401 |
+ inline void send(msg_type_t type, int size, void *buf); |
402 |
}; |
403 |
|
404 |
#endif |
405 |
\ No newline at end of file |