From 7e42ee2003dc566a48a0c44336320d4c164d945e Mon Sep 17 00:00:00 2001 From: Matthew Flatt Date: Fri, 11 Oct 2013 12:31:32 -0600 Subject: [PATCH] fix custodian-related memory-management problem The problem mainly affected `register-custodian-shutdown` from `ffi/unsafe/custodian`, which is used by `math/bigfloat` and `ffi/unsafe/com`. When a value is registered with a custodian, the value is held weakly, but the shutdown procedure is intended to be held strongly. At the C API level, the data associated with a shutdown function pointer is intended to be held strongly. A custodian itself, however, is retained weakly by other custodians in its family, so that custodians can be GCed and their elements transferred to a parent custodian. Since the custodian itself may be held only weakly, the callback & data in a custodian was effectively held weakly --- which, in turn, can break assumptions in code such as `ffi/unsafe/custodian` that expects strong references to prevent finalizers from running. Fix the problem by registering a reference to callback data as data in a custodian's finalizer, which makes the data strongly retained no matter how the custodian is retained. --- .../tests/racket/ffi-custodian.rkt | 38 +++++++++++++++++++ racket/src/racket/src/mzmark_thread.inc | 2 + racket/src/racket/src/mzmarksrc.c | 1 + racket/src/racket/src/schpriv.h | 1 + racket/src/racket/src/thread.c | 13 ++++++- 5 files changed, 54 insertions(+), 1 deletion(-) diff --git a/pkgs/racket-pkgs/racket-test/tests/racket/ffi-custodian.rkt b/pkgs/racket-pkgs/racket-test/tests/racket/ffi-custodian.rkt index eb3ab20a9a..c908e0cf53 100644 --- a/pkgs/racket-pkgs/racket-test/tests/racket/ffi-custodian.rkt +++ b/pkgs/racket-pkgs/racket-test/tests/racket/ffi-custodian.rkt @@ -22,3 +22,41 @@ (unless done? (error "shutdown didn't work")) + +;; ---------------------------------------- +;; Check that shutdown procedure is retained strongly + +(define (go) + (define done? #f) + + (define remembered (make-weak-hasheq)) + (define r-count 0) + (define (remember v) + (hash-set! remembered v #t) + (set! r-count (add1 r-count)) + v) + + (define c (make-custodian)) + (let ([c-sub (parameterize ([current-custodian c]) + (make-custodian))] + [b (box #t)]) + (parameterize ([current-custodian c-sub]) + (void + (register-custodian-shutdown (box 0) + (remember + (lambda (x) + (set! done? (unbox b)))))))) + + (for ([i 2]) + (collect-garbage)) + + (unless (= r-count (hash-count remembered)) + (error 'remembered "gone! ~s" (hash-count remembered))) + + (custodian-shutdown-all c) + + done?) + +(for ([i 10]) + (unless (go) + (error "shutdown failed"))) diff --git a/racket/src/racket/src/mzmark_thread.inc b/racket/src/racket/src/mzmark_thread.inc index 588d77df50..cb3be8505c 100644 --- a/racket/src/racket/src/mzmark_thread.inc +++ b/racket/src/racket/src/mzmark_thread.inc @@ -106,6 +106,7 @@ static int mark_custodian_val_MARK(void *p, struct NewGC *gc) { gcMARK2(m->mrefs, gc); gcMARK2(m->closers, gc); gcMARK2(m->data, gc); + gcMARK2(m->data_ptr, gc); gcMARK2(m->parent, gc); gcMARK2(m->sibling, gc); @@ -127,6 +128,7 @@ static int mark_custodian_val_FIXUP(void *p, struct NewGC *gc) { gcFIXUP2(m->mrefs, gc); gcFIXUP2(m->closers, gc); gcFIXUP2(m->data, gc); + gcFIXUP2(m->data_ptr, gc); gcFIXUP2(m->parent, gc); gcFIXUP2(m->sibling, gc); diff --git a/racket/src/racket/src/mzmarksrc.c b/racket/src/racket/src/mzmarksrc.c index adb33ea7f0..e1d88289b9 100644 --- a/racket/src/racket/src/mzmarksrc.c +++ b/racket/src/racket/src/mzmarksrc.c @@ -1894,6 +1894,7 @@ mark_custodian_val { gcMARK2(m->mrefs, gc); gcMARK2(m->closers, gc); gcMARK2(m->data, gc); + gcMARK2(m->data_ptr, gc); gcMARK2(m->parent, gc); gcMARK2(m->sibling, gc); diff --git a/racket/src/racket/src/schpriv.h b/racket/src/racket/src/schpriv.h index fdf6718cd9..d2697c94e9 100644 --- a/racket/src/racket/src/schpriv.h +++ b/racket/src/racket/src/schpriv.h @@ -665,6 +665,7 @@ struct Scheme_Custodian { Scheme_Custodian_Reference **mrefs; Scheme_Close_Custodian_Client **closers; void **data; + void ***data_ptr; /* points to `data`, registered as finalizer data for strong retention */ /* weak indirections: */ Scheme_Custodian_Reference *parent; diff --git a/racket/src/racket/src/thread.c b/racket/src/racket/src/thread.c index 10a794b586..9976c87f83 100644 --- a/racket/src/racket/src/thread.c +++ b/racket/src/racket/src/thread.c @@ -922,6 +922,7 @@ static void ensure_custodian_space(Scheme_Custodian *m, int k) m->boxes = naya_boxes; m->closers = naya_closers; m->data = naya_data; + *m->data_ptr = naya_data; m->mrefs = naya_mrefs; } } @@ -1082,6 +1083,11 @@ static void adjust_custodian_family(void *mgr, void *skip_move) CUSTODIAN_FAM(r->global_next) = NULL; } +static void do_adjust_custodian_family(void *mgr, void *for_retain) +{ + adjust_custodian_family(mgr, NULL); +} + void insert_custodian(Scheme_Custodian *m, Scheme_Custodian *parent) { /* insert into parent's list: */ @@ -1118,6 +1124,7 @@ Scheme_Custodian *scheme_make_custodian(Scheme_Custodian *parent) { Scheme_Custodian *m; Scheme_Custodian_Reference *mw; + void ***data_ptr; if (!parent) parent = main_custodian; /* still NULL if we're creating main; that's ok */ @@ -1141,9 +1148,12 @@ Scheme_Custodian *scheme_make_custodian(Scheme_Custodian *parent) CUSTODIAN_FAM(m->children) = NULL; + data_ptr = (void ***)scheme_malloc(sizeof(void**)); + m->data_ptr = data_ptr; + insert_custodian(m, parent); - scheme_add_finalizer(m, adjust_custodian_family, NULL); + scheme_add_finalizer(m, do_adjust_custodian_family, data_ptr); return m; } @@ -1417,6 +1427,7 @@ Scheme_Thread *scheme_do_close_managed(Scheme_Custodian *m, Scheme_Exit_Closer_F m->boxes = NULL; m->closers = NULL; m->data = NULL; + *m->data_ptr = NULL; m->mrefs = NULL; m->shut_down = 1;