From 146c7881b757f5623f11b528e5bbb1852bb9231a Mon Sep 17 00:00:00 2001 From: Steffen Heil | secforge Date: Thu, 30 Jul 2026 19:35:59 +0200 Subject: [PATCH 1/2] Fix Debug assert in IntervalTree's is_valid() with til::point keys is_valid() seeded its bounds accumulators with std::numeric_limits ::max()/min(). We instantiate the tree with Scalar = til::point, which has no std::numeric_limits specialization, so the primary template returned a default-constructed til::point{0,0} for *both* sentinels. The right-subtree constraint check then evaluated {0,0} <= center, which holds for essentially any real tree, so is_valid() returned false and assert(is_valid().first) fired. The tree only splits into subtrees once it holds 64 intervals, its default minimum bucket size, so the threshold was exactly 64: Terminal::_getPatterns builds the pattern tree from autodetected URLs over roughly three viewport- heights of buffer, making 64 matches easy to reach. Any Debug build showing a screenful of links aborted. Release builds were unaffected, and the tree itself was structurally sound -- only the checker was wrong. Track emptiness explicitly so no sentinel value is needed. Also accumulate the maximum stop with std::max rather than std::min; upstream's std::min pinned that accumulator at numeric_limits::min() forever, silently reducing both center checks to no-ops, so a numeric_limits specialization alone would have left the check dead. This is the second local deviation from ekg/intervaltree, alongside the struct-key support carried since #7691 (upstream PR ekg/intervaltree#31, still unmerged). Both are now recorded in MAINTAINER_README.md, together with a warning that re-importing the file wholesale breaks the build. Closes #20486 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Pr3nsYBYoJ66jgRidRHyJF --- oss/interval_tree/IntervalTree.h | 51 +++++++++++---- oss/interval_tree/MAINTAINER_README.md | 26 +++++++- src/til/ut_til/IntervalTreeTests.cpp | 62 +++++++++++++++++++ src/til/ut_til/sources | 1 + src/til/ut_til/til.unit.tests.vcxproj | 1 + src/til/ut_til/til.unit.tests.vcxproj.filters | 2 + 6 files changed, 129 insertions(+), 14 deletions(-) create mode 100644 src/til/ut_til/IntervalTreeTests.cpp diff --git a/oss/interval_tree/IntervalTree.h b/oss/interval_tree/IntervalTree.h index e5c82646f97..96af744d125 100644 --- a/oss/interval_tree/IntervalTree.h +++ b/oss/interval_tree/IntervalTree.h @@ -337,43 +337,68 @@ namespace interval_tree const auto minmaxStop = std::minmax_element(intervals.begin(), intervals.end(), IntervalStopCmp()); const auto minmaxStart = std::minmax_element(intervals.begin(), intervals.end(), IntervalStartCmp()); - std::pair> result = { true, { std::numeric_limits::max(), std::numeric_limits::min() } }; + // LOCAL DEVIATION FROM UPSTREAM -- see MAINTAINER_README.md. + // + // result.second is the (minimum start, maximum stop) across this entire subtree. + // Upstream seeds those accumulators with std::numeric_limits::max()/min(), + // which only works for arithmetic Scalars. We instantiate this tree with + // Scalar = til::point, which has no std::numeric_limits specialization, so the + // primary template hands back a default-constructed til::point{0,0} for *both* + // sentinels -- and the ordering checks below then fail for any non-trivial tree. + // Track emptiness explicitly instead so no sentinel value is needed. + // + // Upstream also accumulates the *maximum* stop with std::min, which silently + // reduces the two `center` checks below to no-ops. We use std::max. + std::pair> result = { true, { Scalar{}, Scalar{} } }; + auto empty = true; + const auto merge = [&](const Scalar& start, const Scalar& stop) { + if (empty) + { + result.second = { start, stop }; + empty = false; + } + else + { + result.second.first = std::min(result.second.first, start); + result.second.second = std::max(result.second.second, stop); + } + }; + if (!intervals.empty()) { - result.second.first = std::min(result.second.first, minmaxStart.first->start); - result.second.second = std::min(result.second.second, minmaxStop.second->stop); + merge(minmaxStart.first->start, minmaxStop.second->stop); } if (left) { - auto valid = left->is_valid(); - result.first &= valid.first; - result.second.first = std::min(result.second.first, valid.second.first); - result.second.second = std::min(result.second.second, valid.second.second); - if (!result.first) + const auto valid = left->is_valid(); + if (!valid.first) { + result.first = false; return result; } + // Every interval in the left subtree must stop before center. if (valid.second.second >= center) { result.first = false; return result; } + merge(valid.second.first, valid.second.second); } if (right) { - auto valid = right->is_valid(); - result.first &= valid.first; - result.second.first = std::min(result.second.first, valid.second.first); - result.second.second = std::min(result.second.second, valid.second.second); - if (!result.first) + const auto valid = right->is_valid(); + if (!valid.first) { + result.first = false; return result; } + // Every interval in the right subtree must start after center. if (valid.second.first <= center) { result.first = false; return result; } + merge(valid.second.first, valid.second.second); } if (!std::is_sorted(intervals.begin(), intervals.end(), IntervalStartCmp())) { diff --git a/oss/interval_tree/MAINTAINER_README.md b/oss/interval_tree/MAINTAINER_README.md index edc430dfc1b..bdd8e1e7eee 100644 --- a/oss/interval_tree/MAINTAINER_README.md +++ b/oss/interval_tree/MAINTAINER_README.md @@ -6,11 +6,35 @@ The provenance information (where it came from and which commit) is stored in th Please update the provenance information in that file when ingesting an updated version of the dependent library. That provenance file is automatically read and inventoried by Microsoft systems to ensure compliance with appropriate governance standards. +## Local deviations from upstream + +**`IntervalTree.h` here is _not_ a byte-for-byte copy of upstream.** Do not assume it is. +There are two deliberate deviations, both required because we instantiate the tree with +`Scalar = til::point`, a user-defined struct rather than an arithmetic type: + +1. **Struct keys.** `Scalar` is compared against `Scalar{}` instead of `0` (and `center` is + default-initialized rather than set to `0`). This came in with the original import in + #7691 and corresponds to the still-open upstream pull request + . Without it this file does not compile + against `til::point` at all. +2. **`is_valid()`.** Upstream seeds its bounds accumulators with + `std::numeric_limits::max()`/`::min()`. `til::point` has no `std::numeric_limits` + specialization, so both sentinels come back as a default-constructed `til::point{0,0}` and + the subtree ordering checks fail for any tree large enough to have children — firing + `assert(is_valid().first)` in Debug builds. Upstream additionally accumulates the *maximum* + stop using `std::min`, which silently reduces those same checks to no-ops. Both are fixed + here; see the comment in `is_valid()` and #20486. + +Neither deviation has been accepted upstream, and upstream has had no commit since +2021-03-11, so they are expected to persist. + ## What should be done to update this in the future? 1. Go to the ekg/intervaltree repository on GitHub. 2. Take the file IntervalTree.h wholesale and drop it into the directory here. -3. Don't change anything about it. +3. Don't change anything about it, **except** that you must re-apply the two local deviations + listed above. Taking upstream wholesale without them will break the build, because + `til::point` cannot be compared against `0`. 4. Validate that the license in the root of the repository didn't change and update it if so. It is sitting in the same directory as this readme. If it changed dramatically, ensure that it is still compatible with our license scheme. Also update the NOTICE file in the root of our repository to declare the third-party usage. 5. Submit the pull. diff --git a/src/til/ut_til/IntervalTreeTests.cpp b/src/til/ut_til/IntervalTreeTests.cpp new file mode 100644 index 00000000000..6f2c9ff8966 --- /dev/null +++ b/src/til/ut_til/IntervalTreeTests.cpp @@ -0,0 +1,62 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" + +#include "til/point.h" + +using namespace WEX::Common; +using namespace WEX::Logging; +using namespace WEX::TestExecution; + +// These tests cover our use of oss/interval_tree with a non-arithmetic Scalar. +// See oss/interval_tree/MAINTAINER_README.md for the local deviations involved. +class IntervalTreeTests +{ + TEST_CLASS(IntervalTreeTests); + + using PointTree = interval_tree::IntervalTree; + + // The tree only splits into subtrees once it holds at least 64 intervals (its + // default minimum bucket size), so anything below that never exercised the + // subtree constraints. + static PointTree _makeTree(size_t count) + { + PointTree::interval_vector intervals; + intervals.reserve(count); + for (size_t i = 0; i < count; ++i) + { + const auto y = gsl::narrow(i); + intervals.push_back(PointTree::interval({ 0, y }, { 10, y }, i)); + } + return PointTree{ std::move(intervals) }; + } + + // GH#20486: is_valid() used std::numeric_limits to seed its bounds + // accumulators. til::point has no specialization, so both sentinels were + // til::point{0,0} and this asserted in Debug builds for any tree with 64 or + // more intervals -- i.e. whenever 64+ URLs were autodetected on screen. + TEST_METHOD(IsValidWithSubtrees) + { + for (const size_t count : { 1u, 63u, 64u, 65u, 512u }) + { + const auto tree = _makeTree(count); + VERIFY_IS_TRUE(tree.is_valid().first, NoThrowString().Format(L"%zu intervals", count)); + } + } + + // A tree that reports itself valid must also still find what it contains. + TEST_METHOD(FindsIntervalsAcrossSubtrees) + { + static constexpr size_t count = 200; + const auto tree = _makeTree(count); + + for (size_t i = 0; i < count; ++i) + { + const auto y = gsl::narrow(i); + const auto results = tree.findOverlapping({ 5, y }, { 5, y }); + VERIFY_ARE_EQUAL(1u, results.size(), NoThrowString().Format(L"row %zu", i)); + VERIFY_ARE_EQUAL(i, results.front().value); + } + } +}; diff --git a/src/til/ut_til/sources b/src/til/ut_til/sources index ec02bb53cb0..1be320297b8 100644 --- a/src/til/ut_til/sources +++ b/src/til/ut_til/sources @@ -20,6 +20,7 @@ SOURCES = \ EnumSetTests.cpp \ EnvTests.cpp \ HashTests.cpp \ + IntervalTreeTests.cpp \ MathTests.cpp \ mutex.cpp \ OperatorTests.cpp \ diff --git a/src/til/ut_til/til.unit.tests.vcxproj b/src/til/ut_til/til.unit.tests.vcxproj index e3910f82182..c67036fa302 100644 --- a/src/til/ut_til/til.unit.tests.vcxproj +++ b/src/til/ut_til/til.unit.tests.vcxproj @@ -22,6 +22,7 @@ + diff --git a/src/til/ut_til/til.unit.tests.vcxproj.filters b/src/til/ut_til/til.unit.tests.vcxproj.filters index a3a38f11276..80958390710 100644 --- a/src/til/ut_til/til.unit.tests.vcxproj.filters +++ b/src/til/ut_til/til.unit.tests.vcxproj.filters @@ -12,9 +12,11 @@ + + From c8f28a14fd0dd4fb5c5ad5fa78725e331d24e29f Mon Sep 17 00:00:00 2001 From: Steffen Heil | secforge Date: Fri, 31 Jul 2026 16:52:43 +0200 Subject: [PATCH 2/2] Specialize std::numeric_limits for til types instead of patching interval_tree Per review feedback, revert the change to oss/interval_tree/IntervalTree.h and fix the root assumption on our side instead: is_valid() requires std::numeric_limits to be meaningful, so specialize it for the til coordinate types rather than teaching the vendored file to do without. The vendored header is now byte-identical to what it was before this branch. Also add to LibraryIncludes.h. It was not included anywhere, and relying on a transitive include to declare the primary template before specializing it is fragile. Note this fixes the assert without making is_valid() correct: upstream accumulates the maximum stop with std::min, so that accumulator stays pinned at numeric_limits::min() and the left-subtree check can never fire. The right-subtree check reads the min-start accumulator and works. That trade-off is accepted deliberately -- see the PR discussion. Verified Debug|x64: IntervalTreeTests 3/3, full til suite 281/281. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Pr3nsYBYoJ66jgRidRHyJF --- oss/interval_tree/IntervalTree.h | 51 +++++------------- oss/interval_tree/MAINTAINER_README.md | 73 +++++++++++--------------- src/inc/LibraryIncludes.h | 1 + src/inc/til/point.h | 26 +++++++++ src/inc/til/size.h | 24 +++++++++ src/til/ut_til/IntervalTreeTests.cpp | 21 ++++++-- 6 files changed, 113 insertions(+), 83 deletions(-) diff --git a/oss/interval_tree/IntervalTree.h b/oss/interval_tree/IntervalTree.h index 96af744d125..e5c82646f97 100644 --- a/oss/interval_tree/IntervalTree.h +++ b/oss/interval_tree/IntervalTree.h @@ -337,68 +337,43 @@ namespace interval_tree const auto minmaxStop = std::minmax_element(intervals.begin(), intervals.end(), IntervalStopCmp()); const auto minmaxStart = std::minmax_element(intervals.begin(), intervals.end(), IntervalStartCmp()); - // LOCAL DEVIATION FROM UPSTREAM -- see MAINTAINER_README.md. - // - // result.second is the (minimum start, maximum stop) across this entire subtree. - // Upstream seeds those accumulators with std::numeric_limits::max()/min(), - // which only works for arithmetic Scalars. We instantiate this tree with - // Scalar = til::point, which has no std::numeric_limits specialization, so the - // primary template hands back a default-constructed til::point{0,0} for *both* - // sentinels -- and the ordering checks below then fail for any non-trivial tree. - // Track emptiness explicitly instead so no sentinel value is needed. - // - // Upstream also accumulates the *maximum* stop with std::min, which silently - // reduces the two `center` checks below to no-ops. We use std::max. - std::pair> result = { true, { Scalar{}, Scalar{} } }; - auto empty = true; - const auto merge = [&](const Scalar& start, const Scalar& stop) { - if (empty) - { - result.second = { start, stop }; - empty = false; - } - else - { - result.second.first = std::min(result.second.first, start); - result.second.second = std::max(result.second.second, stop); - } - }; - + std::pair> result = { true, { std::numeric_limits::max(), std::numeric_limits::min() } }; if (!intervals.empty()) { - merge(minmaxStart.first->start, minmaxStop.second->stop); + result.second.first = std::min(result.second.first, minmaxStart.first->start); + result.second.second = std::min(result.second.second, minmaxStop.second->stop); } if (left) { - const auto valid = left->is_valid(); - if (!valid.first) + auto valid = left->is_valid(); + result.first &= valid.first; + result.second.first = std::min(result.second.first, valid.second.first); + result.second.second = std::min(result.second.second, valid.second.second); + if (!result.first) { - result.first = false; return result; } - // Every interval in the left subtree must stop before center. if (valid.second.second >= center) { result.first = false; return result; } - merge(valid.second.first, valid.second.second); } if (right) { - const auto valid = right->is_valid(); - if (!valid.first) + auto valid = right->is_valid(); + result.first &= valid.first; + result.second.first = std::min(result.second.first, valid.second.first); + result.second.second = std::min(result.second.second, valid.second.second); + if (!result.first) { - result.first = false; return result; } - // Every interval in the right subtree must start after center. if (valid.second.first <= center) { result.first = false; return result; } - merge(valid.second.first, valid.second.second); } if (!std::is_sorted(intervals.begin(), intervals.end(), IntervalStartCmp())) { diff --git a/oss/interval_tree/MAINTAINER_README.md b/oss/interval_tree/MAINTAINER_README.md index bdd8e1e7eee..402857f5d63 100644 --- a/oss/interval_tree/MAINTAINER_README.md +++ b/oss/interval_tree/MAINTAINER_README.md @@ -1,41 +1,32 @@ -# Notes for Future Maintainers - -This was originally imported by @PankajBhojwani in September 2020. - -The provenance information (where it came from and which commit) is stored in the file `cgmanifest.json` in the same directory as this readme. -Please update the provenance information in that file when ingesting an updated version of the dependent library. -That provenance file is automatically read and inventoried by Microsoft systems to ensure compliance with appropriate governance standards. - -## Local deviations from upstream - -**`IntervalTree.h` here is _not_ a byte-for-byte copy of upstream.** Do not assume it is. -There are two deliberate deviations, both required because we instantiate the tree with -`Scalar = til::point`, a user-defined struct rather than an arithmetic type: - -1. **Struct keys.** `Scalar` is compared against `Scalar{}` instead of `0` (and `center` is - default-initialized rather than set to `0`). This came in with the original import in - #7691 and corresponds to the still-open upstream pull request - . Without it this file does not compile - against `til::point` at all. -2. **`is_valid()`.** Upstream seeds its bounds accumulators with - `std::numeric_limits::max()`/`::min()`. `til::point` has no `std::numeric_limits` - specialization, so both sentinels come back as a default-constructed `til::point{0,0}` and - the subtree ordering checks fail for any tree large enough to have children — firing - `assert(is_valid().first)` in Debug builds. Upstream additionally accumulates the *maximum* - stop using `std::min`, which silently reduces those same checks to no-ops. Both are fixed - here; see the comment in `is_valid()` and #20486. - -Neither deviation has been accepted upstream, and upstream has had no commit since -2021-03-11, so they are expected to persist. - -## What should be done to update this in the future? - -1. Go to the ekg/intervaltree repository on GitHub. -2. Take the file IntervalTree.h wholesale and drop it into the directory here. -3. Don't change anything about it, **except** that you must re-apply the two local deviations - listed above. Taking upstream wholesale without them will break the build, because - `til::point` cannot be compared against `0`. -4. Validate that the license in the root of the repository didn't change and update it if so. It is sitting in the same directory as this readme. - If it changed dramatically, ensure that it is still compatible with our license scheme. Also update the NOTICE file in the root of our repository to declare the third-party usage. -5. Submit the pull. - +# Notes for Future Maintainers + +This was originally imported by @PankajBhojwani in September 2020. + +The provenance information (where it came from and which commit) is stored in the file `cgmanifest.json` in the same directory as this readme. +Please update the provenance information in that file when ingesting an updated version of the dependent library. +That provenance file is automatically read and inventoried by Microsoft systems to ensure compliance with appropriate governance standards. + +## Local deviation from upstream + +**`IntervalTree.h` here is _not_ a byte-for-byte copy of upstream.** Do not assume it is. +`Scalar` is compared against `Scalar{}` instead of `0`, and `center` is default-initialized +rather than set to `0`, so that a user-defined struct such as `til::point` can be used as a +key. This came in with the original import in #7691 and corresponds to the still-open +upstream pull request . Without it this file +does not compile against `til::point` at all. + +Note that `is_valid()` assumes `std::numeric_limits` is meaningful. We satisfy that +by specializing `std::numeric_limits` for the `til` coordinate types rather than by patching +this file; see `src/inc/til/point.h` and #20486. + +## What should be done to update this in the future? + +1. Go to the ekg/intervaltree repository on GitHub. +2. Take the file IntervalTree.h wholesale and drop it into the directory here. +3. Don't change anything about it, **except** that you must re-apply the local deviation + listed above. Taking upstream wholesale without it will break the build, because + `til::point` cannot be compared against `0`. +4. Validate that the license in the root of the repository didn't change and update it if so. It is sitting in the same directory as this readme. + If it changed dramatically, ensure that it is still compatible with our license scheme. Also update the NOTICE file in the root of our repository to declare the third-party usage. +5. Submit the pull. + diff --git a/src/inc/LibraryIncludes.h b/src/inc/LibraryIncludes.h index 18ad9c6c8c8..aa0191d61b7 100644 --- a/src/inc/LibraryIncludes.h +++ b/src/inc/LibraryIncludes.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include diff --git a/src/inc/til/point.h b/src/inc/til/point.h index 4ac3bd7cfc2..887d03b4893 100644 --- a/src/inc/til/point.h +++ b/src/inc/til/point.h @@ -374,6 +374,32 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" }; } +// til::point is ordered (lexicographically, by y then x), so the extremes are +// well defined. Generic code that needs a "smaller/larger than anything" seed +// value reaches for std::numeric_limits, and the unspecialized primary template +// silently answers til::point{} for both max() and min() -- see GH#20486. +template<> +class std::numeric_limits +{ +public: + static constexpr bool is_specialized = true; + + static constexpr til::point min() noexcept + { + return { til::CoordTypeMin, til::CoordTypeMin }; + } + + static constexpr til::point max() noexcept + { + return { til::CoordTypeMax, til::CoordTypeMax }; + } + + static constexpr til::point lowest() noexcept + { + return min(); + } +}; + #ifdef __WEX_COMMON_H__ namespace WEX::TestExecution { diff --git a/src/inc/til/size.h b/src/inc/til/size.h index 98800ad7539..88a6856ba29 100644 --- a/src/inc/til/size.h +++ b/src/inc/til/size.h @@ -206,6 +206,30 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" } }; +// See the equivalent specialization in point.h. Without this, generic code that +// seeds an accumulator with std::numeric_limits gets til::size{} for both ends. +template<> +class std::numeric_limits +{ +public: + static constexpr bool is_specialized = true; + + static constexpr til::size min() noexcept + { + return { til::CoordTypeMin, til::CoordTypeMin }; + } + + static constexpr til::size max() noexcept + { + return { til::CoordTypeMax, til::CoordTypeMax }; + } + + static constexpr til::size lowest() noexcept + { + return min(); + } +}; + #ifdef __WEX_COMMON_H__ namespace WEX::TestExecution { diff --git a/src/til/ut_til/IntervalTreeTests.cpp b/src/til/ut_til/IntervalTreeTests.cpp index 6f2c9ff8966..0d0f4e3a3c5 100644 --- a/src/til/ut_til/IntervalTreeTests.cpp +++ b/src/til/ut_til/IntervalTreeTests.cpp @@ -32,10 +32,23 @@ class IntervalTreeTests return PointTree{ std::move(intervals) }; } - // GH#20486: is_valid() used std::numeric_limits to seed its bounds - // accumulators. til::point has no specialization, so both sentinels were - // til::point{0,0} and this asserted in Debug builds for any tree with 64 or - // more intervals -- i.e. whenever 64+ URLs were autodetected on screen. + // GH#20486: is_valid() seeds its bounds accumulators from + // std::numeric_limits. Without a specialization the primary template + // answers til::point{0,0} for both ends, which is not a usable sentinel. + TEST_METHOD(NumericLimitsAreUsableSentinels) + { + VERIFY_IS_TRUE(std::numeric_limits::is_specialized); + VERIFY_IS_TRUE(std::numeric_limits::min() < til::point{}); + VERIFY_IS_TRUE(std::numeric_limits::max() > til::point{}); + VERIFY_ARE_EQUAL(std::numeric_limits::min(), std::numeric_limits::lowest()); + + VERIFY_IS_TRUE(std::numeric_limits::is_specialized); + VERIFY_ARE_EQUAL(til::CoordTypeMax, std::numeric_limits::max().width); + VERIFY_ARE_EQUAL(til::CoordTypeMin, std::numeric_limits::min().height); + } + + // Before GH#20486 this asserted in Debug builds for any tree with 64 or more + // intervals -- i.e. whenever 64+ URLs were autodetected on screen. TEST_METHOD(IsValidWithSubtrees) { for (const size_t count : { 1u, 63u, 64u, 65u, 512u })