From 218cae4537ad0983801df06a71bf5f0d83efa720 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Sat, 20 Apr 2024 21:54:12 -0400 Subject: [PATCH 1/4] Remove the interface function fullCollectNoStack from the gcinterface --- druntime/src/core/gc/gcinterface.d | 5 ----- .../core/internal/gc/impl/conservative/gc.d | 21 ------------------- .../src/core/internal/gc/impl/manual/gc.d | 4 ---- druntime/src/core/internal/gc/impl/proto/gc.d | 4 ---- druntime/src/core/internal/gc/proxy.d | 13 +----------- 5 files changed, 1 insertion(+), 46 deletions(-) diff --git a/druntime/src/core/gc/gcinterface.d b/druntime/src/core/gc/gcinterface.d index 5560c6229caf..fa3d859ce361 100644 --- a/druntime/src/core/gc/gcinterface.d +++ b/druntime/src/core/gc/gcinterface.d @@ -53,11 +53,6 @@ interface GC */ void collect() nothrow; - /** - * - */ - void collectNoStack() nothrow; - /** * minimize free space usage */ diff --git a/druntime/src/core/internal/gc/impl/conservative/gc.d b/druntime/src/core/internal/gc/impl/conservative/gc.d index cb8df47507fc..3c2fac2d6391 100644 --- a/druntime/src/core/internal/gc/impl/conservative/gc.d +++ b/druntime/src/core/internal/gc/impl/conservative/gc.d @@ -1252,12 +1252,6 @@ class ConservativeGC : GC } - void collectNoStack() nothrow - { - fullCollectNoStack(); - } - - /** * Begins a full collection, scanning all stack segments for roots. * @@ -1290,21 +1284,6 @@ class ConservativeGC : GC } - /** - * Begins a full collection while ignoring all stack segments for roots. - */ - void fullCollectNoStack() nothrow - { - // Since a finalizer could launch a new thread, we always need to lock - // when collecting. - static size_t go(Gcx* gcx) nothrow - { - return gcx.fullcollect(true, true, true); // standard stop the world - } - runLocked!go(gcx); - } - - /** * Minimize free space usage. */ diff --git a/druntime/src/core/internal/gc/impl/manual/gc.d b/druntime/src/core/internal/gc/impl/manual/gc.d index b820adda1a2b..36add7a2d6af 100644 --- a/druntime/src/core/internal/gc/impl/manual/gc.d +++ b/druntime/src/core/internal/gc/impl/manual/gc.d @@ -79,10 +79,6 @@ class ManualGC : GC { } - void collectNoStack() nothrow - { - } - void minimize() nothrow { } diff --git a/druntime/src/core/internal/gc/impl/proto/gc.d b/druntime/src/core/internal/gc/impl/proto/gc.d index 2286d17d9ce7..dbe86007115e 100644 --- a/druntime/src/core/internal/gc/impl/proto/gc.d +++ b/druntime/src/core/internal/gc/impl/proto/gc.d @@ -76,10 +76,6 @@ class ProtoGC : GC { } - void collectNoStack() nothrow - { - } - void minimize() nothrow { } diff --git a/druntime/src/core/internal/gc/proxy.d b/druntime/src/core/internal/gc/proxy.d index abc8c6ac2ea6..c7dab714e6c5 100644 --- a/druntime/src/core/internal/gc/proxy.d +++ b/druntime/src/core/internal/gc/proxy.d @@ -106,18 +106,7 @@ extern (C) case "none": break; case "collect": - // NOTE: There may be daemons threads still running when this routine is - // called. If so, cleaning memory out from under then is a good - // way to make them crash horribly. This probably doesn't matter - // much since the app is supposed to be shutting down anyway, but - // I'm disabling cleanup for now until I can think about it some - // more. - // - // NOTE: Due to popular demand, this has been re-enabled. It still has - // the problems mentioned above though, so I guess we'll see. - - instance.collectNoStack(); // not really a 'collect all' -- still scans - // static data area, roots, and ranges. + instance.collect(); break; case "finalize": instance.runFinalizers((cast(ubyte*)null)[0 .. size_t.max]); From 782e6ade501e524ea4ee1413795988a216328321 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Sat, 20 Apr 2024 22:08:17 -0400 Subject: [PATCH 2/4] Fix tests that depend on the GC not scanning the stack. --- .../exceptions/src/invalid_memory_operation.d | 3 ++- druntime/test/valgrind/src/no_use_after_gc.d | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/druntime/test/exceptions/src/invalid_memory_operation.d b/druntime/test/exceptions/src/invalid_memory_operation.d index 1cf1a9fd4ba1..bd30eebe4257 100644 --- a/druntime/test/exceptions/src/invalid_memory_operation.d +++ b/druntime/test/exceptions/src/invalid_memory_operation.d @@ -8,5 +8,6 @@ struct S void main() { - new S; + foreach(i; 0 .. 100) + new S; } diff --git a/druntime/test/valgrind/src/no_use_after_gc.d b/druntime/test/valgrind/src/no_use_after_gc.d index 1bb45628ff09..527af535d52b 100644 --- a/druntime/test/valgrind/src/no_use_after_gc.d +++ b/druntime/test/valgrind/src/no_use_after_gc.d @@ -16,7 +16,7 @@ struct S __gshared int result; // Trick the optimizer -int main() +int makeBadInstances() { auto a = new S; auto b = new S; @@ -24,3 +24,16 @@ int main() b.other = a; return result; } + +int destroyStack() +{ + int[1000] x = result; + return x[123]; +} + +int main() +{ + int x = makeBadInstances(); + x += destroyStack(); + return x; +} From 9cc44ed5237a04cb17c7ca8d292ee0c3aedbdff3 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Sun, 21 Apr 2024 21:15:42 -0400 Subject: [PATCH 3/4] Remove all nostack remnants --- .../core/internal/gc/impl/conservative/gc.d | 52 ++++++++----------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/druntime/src/core/internal/gc/impl/conservative/gc.d b/druntime/src/core/internal/gc/impl/conservative/gc.d index 3c2fac2d6391..b1b270799aa7 100644 --- a/druntime/src/core/internal/gc/impl/conservative/gc.d +++ b/druntime/src/core/internal/gc/impl/conservative/gc.d @@ -2535,14 +2535,11 @@ struct Gcx } // collection step 2: mark roots and heap - void markAll(alias markFn)(bool nostack) nothrow + void markAll(alias markFn)() nothrow { - if (!nostack) - { - debug(COLLECT_PRINTF) printf("\tscan stacks.\n"); - // Scan stacks and registers for each paused thread - thread_scanAll(&markFn); - } + debug(COLLECT_PRINTF) printf("\tscan stacks.\n"); + // Scan stacks registers, and TLS for each paused thread + thread_scanAll(&markFn); // Scan roots[] debug(COLLECT_PRINTF) printf("\tscan roots[]\n"); @@ -2563,14 +2560,11 @@ struct Gcx } version (COLLECT_PARALLEL) - void collectAllRoots(bool nostack) nothrow + void collectAllRoots() nothrow { - if (!nostack) - { - debug(COLLECT_PRINTF) printf("\tcollect stacks.\n"); - // Scan stacks and registers for each paused thread - thread_scanAll(&collectRoots); - } + debug(COLLECT_PRINTF) printf("\tcollect stacks.\n"); + // Scan stacks registers and TLS for each paused thread + thread_scanAll(&collectRoots); // Scan roots[] debug(COLLECT_PRINTF) printf("\tcollect roots[]\n"); @@ -2899,7 +2893,7 @@ struct Gcx } version (COLLECT_FORK) - ChildStatus markFork(bool nostack, bool block, bool doParallel) nothrow + ChildStatus markFork(bool block, bool doParallel) nothrow { // Forking is enabled, so we fork() and start a new concurrent mark phase // in the child. If the collection should not block, the parent process @@ -2915,11 +2909,11 @@ struct Gcx int child_mark() scope { if (doParallel) - markParallel(nostack); + markParallel(); else if (ConservativeGC.isPrecise) - markAll!(markPrecise!true)(nostack); + markAll!(markPrecise!true)(); else - markAll!(markConservative!true)(nostack); + markAll!(markConservative!true)(); return 0; } @@ -2978,11 +2972,11 @@ struct Gcx // do the marking in this thread disableFork(); if (doParallel) - markParallel(nostack); + markParallel(); else if (ConservativeGC.isPrecise) - markAll!(markPrecise!false)(nostack); + markAll!(markPrecise!false)(); else - markAll!(markConservative!false)(nostack); + markAll!(markConservative!false)(); } else { assert(r == ChildStatus.done); assert(r != ChildStatus.running); @@ -2995,7 +2989,7 @@ struct Gcx * Return number of full pages free'd. * The collection is done concurrently only if block and isFinal are false. */ - size_t fullcollect(bool nostack = false, bool block = false, bool isFinal = false) nothrow + size_t fullcollect(bool block = false, bool isFinal = false) nothrow { // It is possible that `fullcollect` will be called from a thread which // is not yet registered in runtime (because allocating `new Thread` is @@ -3077,7 +3071,7 @@ Lmark: { version (COLLECT_FORK) { - auto forkResult = markFork(nostack, block, doParallel); + auto forkResult = markFork(block, doParallel); final switch (forkResult) { case ChildStatus.error: @@ -3104,14 +3098,14 @@ Lmark: else if (doParallel) { version (COLLECT_PARALLEL) - markParallel(nostack); + markParallel(); } else { if (ConservativeGC.isPrecise) - markAll!(markPrecise!false)(nostack); + markAll!(markPrecise!false)(); else - markAll!(markConservative!false)(nostack); + markAll!(markConservative!false)(); } thread_processGCMarks(&isMarked); @@ -3163,7 +3157,7 @@ Lmark: updateCollectThresholds(); if (doFork && isFinal) - return fullcollect(true, true, false); + return fullcollect(true, false); return freedPages; } @@ -3279,10 +3273,10 @@ Lmark: shared uint stoppedThreads; bool stopGC; - void markParallel(bool nostack) nothrow + void markParallel() nothrow { toscanRoots.clear(); - collectAllRoots(nostack); + collectAllRoots(); if (toscanRoots.empty) return; From 4eff190c078b1259c7475b6995e37c3226a4dce5 Mon Sep 17 00:00:00 2001 From: Steven Schveighoffer Date: Mon, 22 Apr 2024 21:25:38 -0400 Subject: [PATCH 4/4] Add changelog entry --- changelog/druntime.removenostackcollect.dd | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 changelog/druntime.removenostackcollect.dd diff --git a/changelog/druntime.removenostackcollect.dd b/changelog/druntime.removenostackcollect.dd new file mode 100644 index 000000000000..b29cade4811a --- /dev/null +++ b/changelog/druntime.removenostackcollect.dd @@ -0,0 +1,38 @@ +Remove all `collectNoStack` functions and API from druntime. + +The function `collectNoStack` in the D garbage collector performed a +collection, without using any roots from thread stacks or thread-local-storage. +The danger of running this mechanism is that any blocks of memory which only +have a reference from a thread might be collected, while the thread is still +running and possibly using the memory. + +The only time this function was called was at GC termination. At GC +termination, the GC is about to be destroyed, and so we want to run as many +destructors as possible. However, if some thread is using GC-allocated memory, +cleaning up that memory isn't going to help matters. Either it will crash after +the GC cleans the memory, or it will crash after the GC is destroyed. + +The original purpose of this function (from D1) was to ensure simple uses of +the GC were cleaned up in small test programs, as this mechanism was only used +on single-threaded programs (and of course, at program exit). Also note at the +time, D1 was 32-bit, and false pointers where much more common. Avoiding +scanning stacks would aid in avoiding seemingly random behavior in cleanup. +However, as shown below, there are more deterministic ways to ensure data is +always cleaned up. + +Today, the dangers are much greater that such a function is even callable -- +any call to such a function would immediately start use-after-free memory +corruption in any thread that is still running. Therefore, we are removing the +functionality entirely, and simply doing a standard GC cleanup (scanning stacks +and all). One less footgun is the benefit for having less guaranteed GC clean +up at program exit. + +In addition, the GC today is a bit smarter about where the valid stack is, so +there is even less of a chance of leaving blocks unfinalized. + +As always, the GC is *not* guaranteed to clean up any block at the end of +runtime. Any change in behavior with code that had blocks clean up before, but +no longer are cleaned up is still within specification. And if you want the +behavior that absolutely cleans all blocks, you can use the +`--DRT-gcopt=cleanup:finalize` druntime configuration option, which will clean +up all blocks without even scanning.