8000 py/malloc: Fix deferred gc of tracked frees called from finalisers. · micropython/micropython@d2b8f2f · GitHub
[go: up one dir, main page]
Skip to content

Commit d2b8f2f

Browse files
projectgusprojectgus
committed
py/malloc: Fix deferred gc of tracked frees called from finalisers.
When a node in a tracked allocation (such as mbedTLS buffer data) is freed from a finaliser (such as TLS socket object), the memory can't be immediately freed as GC is locked and was being left for the next GC. However this meant an allocation could fail, even though it would have succeeded after the next collection. Fix the problem by adding "pending free" tracked allocations to their own linked list, and cleaning any up at the end of the GC pass. Necessary for extmod/ssl_noleak test to pass on RPI_PICO_W and possibly other ports with MICROPY_MBEDTLS_CONFIG_BARE_METAL set. Includes a specific coverage test for the unix port. Fixes an issue originally reported on Discord where reconnecting to an mqtt server with umqtt.simple and SSL would fail on rp2 with OSError(ENOMEM). This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
1 parent 5799daf commit d2b8f2f

File tree

5 files changed

+122
-15
lines changed

5 files changed

+122
-15
lines changed

‎ports/unix/coverage.c

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,36 @@ static MP_DEFINE_CONST_OBJ_TYPE(
150150
static const mp_obj_str_t str_no_hash_obj = {{&mp_type_str}, 0, 10, (const byte *)"0123456789"};
151151
static const mp_obj_str_t bytes_no_hash_obj = {{&mp_type_bytes}, 0, 10, (const byte *)"0123456789"};
152152

153+
#if MICROPY_ENABLE_FINALISER && MICROPY_TRACKED_ALLOC
154+
// Dummy class which contains a tracked allocation freed from its finaliser
155+
extern const mp_obj_type_t tracked_container_type;
156+
157+
typedef struct {
158+
mp_obj_base_t base;
159+
void *inner_buf;
160+
} obj_tracked_container_t;
161+
162+
static mp_obj_t tracked_container_del(mp_obj_t obj) {
163+
obj_tracked_container_t *container = MP_OBJ_TO_PTR(obj);
164+
m_tracked_free(container->inner_buf);
165+
container->inner_buf = NULL;
166+
return mp_const_none;
167+
}
168+
MP_DEFINE_CONST_FUN_OBJ_1(tracked_container_del_obj, tracked_container_del);
169+
170+
static const mp_rom_map_elem_t tracked_container_locals_dict_table[] = {
171+
{ MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&tracked_container_del_obj) },
172+
};
173+
static MP_DEFINE_CONST_DICT(tracked_container_locals_dict, tracked_container_locals_dict_table);
174+
175+
MP_DEFINE_CONST_OBJ_TYPE(
176+
tracked_container_type,
177+
MP_QSTR_TrackedContainer,
178+
MP_TYPE_FLAG_NONE,
179+
locals_dict, &tracked_container_locals_dict
180+
);
181+
#endif
182+
153183
// Corrupt a pointer in a way that compiler can't optimise
154184
// the reversible operation out
155185
static void *MP_NOINLINE coverage_flip_ptr(void *ptr) {
@@ -319,6 +349,39 @@ static mp_obj_t extra_coverage(void) {
319349
mp_printf(&mp_plat_print, "m_tracked_head = %p\n", MP_STATE_VM(m_tracked_head));
320350
}
321351

352+
// tracked allocation with finalisers
353+
#if MICROPY_ENABLE_FINALISER && MICROPY_TRACKED_ALLOC
354+
{
355+
const size_t INNER_SZ = 1024;
356+
obj_tracked_container_t *c = mp_obj_malloc_with_finaliser(obj_tracked_container_t,
357+
&tracked_container_type);
358+
c->inner_buf = m_tracked_calloc(INNER_SZ, 1);
359+
assert(c->inner_buf != NULL);
360+
361+
// Measure current heap allocation
362+
gc_collect();
363+
size_t before = m_get_current_bytes_allocated();
364+
365+
// hide the container object from the GC and collect again
366+
c = coverage_flip_ptr(c);
367+
gc_collect();
368+
369+
// Note: the asserts below will fail if a copy of the pointer 'c' ends
370+
// up in a stale register or stack address that isn't "hidden" via
371+
// coverage_flip_ptr(), so the object remains accessible. This is
372+
// something a C compiler can't promise not to do, but currently it
373+
// doesn't seem to happen.
374+
375+
// Check the finaliser ran immediately
376+
c = coverage_flip_ptr(c);
377+
assert(c->inner_buf == NULL);
378+
379+
// Check the inner buffer was immediately freed
380+
size_t after = m_get_current_bytes_allocated();
381+
assert(after <= before - INNER_SZ); // Not exact as tracked alloc node adds overhead
382+
}
383+
#endif // MICROPY_ENABLE_FINALISER && MICROPY_TRACKED_ALLOC
384+
322385
// vstr
323386
{
324387
mp_printf(&mp_plat_print, "# vstr\n");

‎py/gc.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -641,6 +641,9 @@ void gc_collect_end(void) {
641641
}
642642
MP_STATE_THREAD(gc_lock_depth)--;
643643
GC_EXIT();
644+
#if MICROPY_TRACKED_ALLOC && MICROPY_ENABLE_FINALISER
645+
m_tracked_free_pending();
646+
#endif
644647
}
645648

646649
void gc_sweep_all(void) {

‎py/malloc.c

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,15 @@ static size_t m_tracked_count_links(size_t *nb) {
238238
}
239239
#endif
240240

241+
static void m_tracked_node_insert(m_tracked_node_t **head, m_tracked_node_t *node) {
242+
if ((*head) != NULL) {
243+
(*head)->prev = node;
244+
}
245+
node->prev = NULL;
246+
node->next = (*head);
247+
(*head) = node;
248+
}
249+
241250
void *m_tracked_calloc(size_t nmemb, size_t size) {
242251
m_tracked_node_t *node = m_malloc_maybe(sizeof(m_tracked_node_t) + nmemb * size);
243252
if (node == NULL) {
@@ -248,12 +257,7 @@ void *m_tracked_calloc(size_t nmemb, size_t size) {
248257
size_t n = m_tracked_count_links(&nb);
249258
DEBUG_printf("m_tracked_calloc(%u, %u) -> (%u;%u) %p\n", (int)nmemb, (int)size, (int)n, (int)nb, node);
250259
#endif
251-
if (MP_STATE_VM(m_tracked_head) != NULL) {
252-
MP_STATE_VM(m_tracked_head)->prev = node;
253-
}
254-
node->prev = NULL;
255-
node->next = MP_STATE_VM(m_tracked_head);
256-
MP_STATE_VM(m_tracked_head) = node;
260+
m_tracked_node_insert(&MP_STATE_VM(m_tracked_head), node);
257261
#if MICROPY_TRACKED_ALLOC_STORE_SIZE
258262
node->size = nmemb * size;
259263
#endif
@@ -263,6 +267,18 @@ void *m_tracked_calloc(size_t nmemb, size_t size) {
263267
return &node->data[0];
264268
}
265269

270+
static void m_tracked_inner_free_node(m_tracked_node_t *node) {
271+
m_free(node
272+
#if MICROPY_MALLOC_USES_ALLOCATED_SIZE
273+
#if MICROPY_TRACKED_ALLOC_STORE_SIZE
274+
, node->size
275+
#else
276+
, gc_nbytes(node)
277+
#endif
278+
#endif
279+
);
280+
}
281+
266282
void m_tracked_free(void *ptr_in) {
267283
if (ptr_in == NULL) {
268284
return;
@@ -287,16 +303,33 @@ void m_tracked_free(void *ptr_in) {
287303
} else {
288304
MP_STATE_VM(m_tracked_head) = node->next;
289305
}
290-
m_free(node
291-
#if MICROPY_MALLOC_USES_ALLOCATED_SIZE
292-
#if MICROPY_TRACKED_ALLOC_STORE_SIZE
293-
, node->size
294-
#else
295-
, gc_nbytes(node)
296-
#endif
297-
#endif
298-
);
306+
#if MICROPY_ENABLE_FINALISER
307+
if (MP_STATE_THREAD(gc_lock_depth) == 0) {
308+
m_tracked_inner_free_node(node);
309+
} else {
310+
// If GC is already locked (i.e. we're being called from a finaliser)
311+
// then can't free immediately. Add the node to a list to be explicitly
312+
// freed after GC is finished, so it doesn't have to wait until the next GC
313+
// pass.
314+
m_tracked_node_insert(&MP_STATE_VM(m_tracked_pending_free_head), node);
315+
}
316+
#else
317+
m_tracked_inner_free_node(node);
318+
#endif // MICROPY_ENABLE_FINALISER
319+
}
320+
321+
#if MICROPY_ENABLE_FINALISER
322+
void m_tracked_free_pending(void) {
323+
m_tracked_node_t *head;
324+
325+
if (MP_STATE_THREAD(gc_lock_depth) == 0) {
326+
while ((head = MP_STATE_VM(m_tracked_pending_free_head))) {
327+
MP_STATE_VM(m_tracked_pending_free_head) = head->next;
328+
m_tracked_inner_free_node(head);
329+
}
330+
}
299331
}
332+
#endif // MICROPY_ENABLE_FINALISER
300333

301334
#endif // MICROPY_TRACKED_ALLOC
302335

‎py/misc.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,11 @@ NORETURN void m_malloc_fail(size_t num_bytes);
112112
// them. They can be used by code that requires traditional C malloc/free semantics.
113113
void *m_tracked_calloc(size_t nmemb, size_t size);
114114
void m_tracked_free(void *ptr_in);
115+
116+
#if MICROPY_ENABLE_FINALISER
117+
// Internal function. Called after gc_sweep() to actually free any nodes freed from finalisers.
118+
void m_tracked_free_pending(void);
119+
#endif
115120
#endif
116121

117122
#if MICROPY_MEM_STATS

‎py/mpstate.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ typedef struct _mp_state_vm_t {
151151

152152
#if MICROPY_TRACKED_ALLOC
153153
struct _m_tracked_node_t *m_tracked_head;
154+
#if MICROPY_ENABLE_FINALISER
155+
struct _m_tracked_node_t *m_tracked_pending_free_head;
156+
#endif
154157
#endif
155158

156159
// non-heap memory for creating an exception if we can't allocate RAM

0 commit comments

Comments
 (0)
0