-
Notifications
You must be signed in to change notification settings - Fork 1.6k
STL Hardening #5274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
STL Hardening #5274
Conversation
Need to silence Clang -Wunused-variable warnings in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work. I'm excited for💎hardening💎 to help improve the correctness of C++ programs everywhere.
I left some comments, most nitpicks, some questions. No major blockers, but I'd like to resolve these before approving. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No concerns from me in moving forward, LGTM. I'll be monitoring for more reviews in this thread in case any of them pick up on something I missed.
Before enabling this by default (in another PR), I think it would be wise to benchmark. 🔬 . Those numbers may make for a good blog post!
(I won't be moving this through the kanban board as I understand you're waiting for even more approvals from other stakeholders)
|
WG21-P3471R4 was just voted into the Working Paper. R3 added As a result, I have no further changes for this PR. |
… tests as SKIPPED.
|
I've pushed an additional commit to pre-emptively resolve a merge conflict with the toolset update #5284. (When mirroring, I always sequence toolset updates first, so all following PRs run on the new pool.) The toolset update entirely SKIPPED the |
|
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
🗺️ Overview
This was preceded by the "prelude" PRs #5255 and #5270, but they aren't critical for understanding this PR.
Resolves #5090.
After this, I'll work on a distinct-but-related feature, "destructor tombstones".
See the STL Hardening wiki page for details, most of which I won't repeat here. The highlights are:
_CONTAINER_DEBUG_LEVEL(aka CDL)._ITERATOR_DEBUG_LEVEL(aka IDL).I don't have concrete numbers about the performance impact of these changes, especially in real-world programs. (Remember, I write one-page programs named
meow.cppall day.) Microbenchmarks aren't terribly illuminating for this, so I'm intentionally not adding any. We've gathered a set of MS-internal customers who have promised to be early adopters and provide performance feedback; we also welcome external feedback.For programs where STL Hardening is disabled (which is all of them, until users take action to opt-in, for the time being), there is zero performance impact in release mode - the checks are all preprocessed out. And in debug mode (where our long-standing IDL=2 debug checks supersede STL Hardening), this work led me to improve the non-optimized codegen for debug checks - that was in #5270.
This has been approved by my bosses and boss-like entities, but not my ultrabosses (apparently I worked a bit faster than expected), so while I'm getting this out for review as soon as possible, I'll wait for their blessings before merging this.
As usual for my PRs, this is organized into well-structured commits - let's look at them. Make rocket go now.
💀 Remove CDL
In these commits, I'm taking the legacy CDL checks that we don't want to be part of STL Hardening, and changing them from testing
#if _CONTAINER_DEBUG_LEVEL > 0to#if _ITERATOR_DEBUG_LEVEL != 0. It helps to understand that CDL was only ever 0 or 1, and (because nobody ever bothered to use it), it was defined as:STL/stl/inc/yvals.h
Lines 160 to 166 in d43d49a
That is, for IDL=0 (the release mode default) we defined CDL to be 0, and for IDL != 0 (including IDL=2, the debug mode default) we defined CDL to be 1.
So this replacement isn't altering when the checks are emitted, as long as nobody was defining CDL, which they weren't (outside of our test suite).
valarray's 28 binary ops, no test impact.condition_variable::wait_until, no test impact.basic_string::resize_and_overwrite, no test impact.basic_string_view's ctor, update test coverage.span::size_bytes, update test coverage.<mdspan>, update test coverage.-Wunused-variablewarnings in the tests.<generator>, update test coverage.iota_view, update test coverage.repeat_view, update test coverage.filter_view, update test coverage.take_view, update test coverage.take_while_view, update test coverage.drop_view, update test coverage.drop_while_view, update test coverage.views::counted, update test coverage.chunk_view, update test coverage.slide_view, update test coverage.chunk_by_view, update test coverage.stride_view, update test coverage.cartesian_product_view, update test coverage.☑️ Control macros
Unlike CDL's weird system which was entangled with IDL, STL Hardening has a simpler, yet more powerful scheme.
We have a coarse-grained control macro,
_MSVC_STL_HARDENING, which should be either0or1. The default value is currently0.We then have fine-grained control macros, which are per-class. For example,
_MSVC_STL_HARDENING_VECTORcontrols whethervector(bothvector<T>andvector<bool>) are hardened. The default value is whatever the coarse-grained macro is.This allows the user to have a one-touch setting to control the whole scheme. But if they want, they can add per-class control. (For example, disabled in general but enabled for just
spanas an initial test of perf impact. Or, enabled in general but disabled for justbasic_string.)As for all control macros, if the user wishes to provide a definition, they should do this project-wide, i.e. on the command line or in their build system, not in source files.
_MSVC_STL_HARDENINGcontrol macros, initially off-by-default.💎 Add hardening checks
After all of this buildup, the changes to add hardening are generally quite simple, because we already had most of the checks for CDL. The pattern is that (e.g. in
<array>)#if _CONTAINER_DEBUG_LEVEL > 0is being replaced by#if _MSVC_STL_HARDENING_ARRAY || _ITERATOR_DEBUG_LEVEL != 0. That's it - the actual check below remains unchanged.I've previously explained how converting a CDL check into IDL != 0 is behavior-preserving (for everyone who didn't touch CDL). This simply adds the fine-grained macro
_MSVC_STL_HARDENING_ARRAYas an additional way to enable the check.This "logical OR" pattern, as well as the unified nature of the checking macros (
_STL_VERIFYetc.) makes the interaction between STL Hardening and IDL easy to understand:In a few of these commits, I had to add checking that was outright missing, or not controlled by CDL, or was entangled with normal control flow. To support "Continue on Error" (more on that later), our new pattern is that checks should always be performed prior to library logic, and not interact with it.
array.deque, overhaulpop_front/pop_back.forward_list, addpop_front.list.vector, addvector<bool>::pop_back.vector<bool>::pop_back. I believe IDL=2 debug checks would have caught it, but with a worse message (because it would be detected when formingend() - 1).basic_string, addpop_back.basic_string_view.span.mdspan.expected.optional.valarray.ranges::view_interface.bitset, overhaulingoperator[].<bitset>:operator[]could be SAL annotated with_In_range_(<, _Bits)#5262 for followup._Validate()intooperator[]fixes some libcxxbitsettests that were failing due toconstexprstep limits. (In a later commit, I've copied Toolset update: VS 2022 17.14 Preview 1 #5284's change to entirely skip thosebitsettests, which are too unreliable.)☠️ Removing CDL from human memory
VSO_0677157_flist_merge_edge_cases, avoid duplicate coverage offront().VSO_0000000_string_view_idl, remove comment citing VSO-830211, the bug that requested CDL (added by MSVC-PR-174936 on 2019-04-15).😸 Test everything
This tests every single hardened check. I've intentionally avoided extracting commonality into template machinery - there just isn't enough of it to be worth the added complexity. It's much easier to read 2-3 lines and see what each test function is doing.
GH_005090_stl_hardening🎁 Bonus enhancement:
span::size_bytes()debug checksWhile reviewing the prelude PR, @sivadeilra observed that this could be improved: #5270 (comment)
It was too much effort to move into the prelude PR. This isn't strictly part of hardening, but it's related to debug checks, so it's here.
span::size_bytes()debug checks intospan's ctors.size_bytes()explaining that we've already checked. (Having constructor checks is sufficient becausespans can't grow.)_Span_extent_type, which has different cases for static vs. dynamic extents. We need to saysizeof(_Ty)because we don't have theelement_typetypedef.std::numeric_limits"; the name is obvious, and if we can mentionspanunqualified, surelynumeric_limitsis fine.static_assert, as the throughput cost is trivial. I'm doing this in the constructor, (1) for consistency with dynamic extents below, and (2) because I could imagine pathological code (in STL test suites?) claiming thatspan<int, static_cast<size_t>(-2)>isn't forbidden to simply instantiate as a type - but the moment that an object of such a type is constructed, the constructor arguments must surely be lying, so we're justified in performing a debug check then._Span_extent_typeconstruction performs the runtime check. This is IDL != 0 (as opposed to hardening), so it's okay if we check more frequently than absolutely necessary. (For example, when constructing from a built-in array or astd::array, we don't need to worry about overflow.)🦹♂️ DOOM RULES ALL
Finally, I'm overhauling how the STL's termination mechanism works. Our cool debug assert messages are unchanged (that's controlled by
_RPTF0(_CRT_ASSERT, mesg);in_STL_REPORT_ERROR), but I'm changing what happens when we need to end program execution._MSVC_STL_DOOM_FUNCTIONis so named because it could expand to many things: a user-customized function,abort(),_invoke_watson(), or (in the future)__fastfail()._MSVC_STL_USE_ABORT_AS_DOOM_FUNCTIONas users have specifically requestedabort()._STL_CRT_SECURE_INVALID_PARAMETER(expr)parameter to_MSVC_STL_DOOM_FUNCTION(mesg). We always call it with amesgstring, so stringizing it again with#expris a mistake._invoke_watson()will immediately call__fastfail()(on Win8+), without calling any user-customized invalid parameter handlers. This gives attackers fewer opportunities to exploit running code._SECURE_SCLera of 2005, the CRT's invalid parameter handler was considered a desirable path for termination. We've got new guidance from our internal teams that fastfail is the best path, which makes a lot of sense.__fastfail(), resulting in less codegen (I suspect pushing all of thenullptr/0arguments onto the stack is a cost)._invoke_watsonin the no-exceptions definition of_RAISE, for consistency./D_STL_CALL_ABORT_INSTEAD_OF_INVALID_PARAMETERfrom the test suites.