From 9e78de4d86ec1b61cc39ed90083373bfa111ccdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Ludwig?= Date: Fri, 20 Sep 2024 11:07:22 +0200 Subject: [PATCH 1/2] Fix Bugzilla 24773: Don't invoke destructors on uninitialized elements in stable sort Uses a regular initialized temporary array when sorting elements with an elaborate assignment to avoid undefined behavior when destructors, postblits or copy constructors are invoked during the array assignment. --- std/algorithm/sorting.d | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/std/algorithm/sorting.d b/std/algorithm/sorting.d index c5b085d1037..3381194e849 100644 --- a/std/algorithm/sorting.d +++ b/std/algorithm/sorting.d @@ -2385,7 +2385,11 @@ private template TimSortImpl(alias pred, R) size_t stackLen = 0; // Allocate temporary memory if not provided by user - if (temp.length < minTemp) temp = () @trusted { return uninitializedArray!(T[])(minTemp); }(); + if (temp.length < minTemp) + { + static if (hasElaborateAssign!T) temp = new T[](minTemp); + else temp = () @trusted { return uninitializedArray!(T[])(minTemp); }(); + } for (size_t i = 0; i < range.length; ) { @@ -3076,6 +3080,20 @@ private template TimSortImpl(alias pred, R) array.sort!("a < b", SwapStrategy.stable); } +// https://issues.dlang.org/show_bug.cgi?id=24773 +@safe unittest +{ + static struct S + { + int i = 42; + ~this() { assert(i == 42); } + } + + auto array = new S[](400); + array.sort!((a, b) => false, SwapStrategy.stable); +} + + // schwartzSort /** Alternative sorting method that should be used when comparing keys involves an From ef6a991534e085d82eebc65a9b09af68c05b0906 Mon Sep 17 00:00:00 2001 From: Jonathan M Davis Date: Thu, 17 Oct 2024 21:00:02 -0600 Subject: [PATCH 2/2] Fix bugzilla issue 24809: In some cases, stable sort assigns to unininitialized elements (#9057) --- std/algorithm/sorting.d | 73 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/std/algorithm/sorting.d b/std/algorithm/sorting.d index 3381194e849..09f02e0e921 100644 --- a/std/algorithm/sorting.d +++ b/std/algorithm/sorting.d @@ -2625,11 +2625,21 @@ private template TimSortImpl(alias pred, R) // can't use `temp.length` if there's no default constructor static if (__traits(compiles, { T defaultConstructed; cast(void) defaultConstructed; })) { - if (__ctfe) temp.length = newSize; - else temp = () @trusted { return uninitializedArray!(T[])(newSize); }(); + + static if (hasElaborateAssign!T) + temp.length = newSize; + else + { + if (__ctfe) temp.length = newSize; + else temp = () @trusted { return uninitializedArray!(T[])(newSize); }(); + } } else { + static assert(!hasElaborateAssign!T, + "Structs which have opAssign but cannot be default-initialized " ~ + "do not currently work with stable sort: " ~ + "https://issues.dlang.org/show_bug.cgi?id=24810"); temp = () @trusted { return uninitializedArray!(T[])(newSize); }(); } } @@ -3093,6 +3103,65 @@ private template TimSortImpl(alias pred, R) array.sort!((a, b) => false, SwapStrategy.stable); } +// https://issues.dlang.org/show_bug.cgi?id=24809 +@safe unittest +{ + static struct E + { + int value; + int valid = 42; + + ~this() + { + assert(valid == 42); + } + } + + import std.array : array; + import std.range : chain, only, repeat; + auto arr = chain(repeat(E(41), 18), + only(E(39)), + repeat(E(41), 16), + only(E(1)), + repeat(E(42), 33), + only(E(33)), + repeat(E(42), 16), + repeat(E(43), 27), + only(E(33)), + repeat(E(43), 34), + only(E(34)), + only(E(43)), + only(E(63)), + repeat(E(44), 42), + only(E(27)), + repeat(E(44), 11), + repeat(E(45), 64), + repeat(E(46), 3), + only(E(11)), + repeat(E(46), 7), + only(E(4)), + repeat(E(46), 34), + only(E(36)), + repeat(E(46), 17), + repeat(E(47), 36), + only(E(39)), + repeat(E(47), 26), + repeat(E(48), 17), + only(E(21)), + repeat(E(48), 5), + only(E(39)), + repeat(E(48), 14), + only(E(58)), + repeat(E(48), 24), + repeat(E(49), 13), + only(E(40)), + repeat(E(49), 38), + only(E(18)), + repeat(E(49), 11), + repeat(E(50), 6)).array(); + + arr.sort!((a, b) => a.value < b.value, SwapStrategy.stable)(); +} // schwartzSort /**