Repository navigation
Hunting down a UB exploit under LTO lead to the following: - #1015
Merged
Merged
Conversation
quickbeam123
added this pull request to stack #1016
October 9, 2026 17:40
MichaelRawson
approved these changes
Oct 10, 2026
MichaelRawson
left a comment
MichaelRawson
left a comment
Contributor
There was a problem hiding this comment.
I'm not sure I'd do the SkipList thing without a wider mandate for this kind of thing throughout. But it's a matter of taste, and the rest looks good. Thanks for hunting this nightmare bug!
MichaelRawson
force-pushed
the
lto-mystery
branch
from
October 10, 2026 12:49
694196a to
5c93b8b
Compare
quickbeam123
force-pushed
the
lto-mystery
branch
from
October 10, 2026 19:31
5c93b8b to
19fa315
Compare
Coproduct kept the live alternative in `char _content[maxSize]` and read it
back through
template<class B> B& cast() { return *(B*)_content; }
which every payload access in the class funnels through (unwrap, and so
compare, operator==, apply, operator<<). That is undefined twice over:
- the returned pointer is derived from the char array, not from the object
that reused its storage, which is exactly the case std::launder exists for;
- the same bytes are written at one type (placement-new of the alternative,
or a memberwise copy of the Coproduct) and read at another, which is the
assumption -fstrict-aliasing is allowed to rely on being false.
Replaced by a recursive union, so the compiler is told the truth -- these types
share this memory -- and reading the live alternative is an ordinary member
access at the right type. No cast, nothing to launder, and the union supplies
the size and alignment, so MaxSize/MaxAlign are gone.
cast<B>() becomes castAt<idx>(). All five call sites already had the index in
hand, and going through it means the access path used to read an alternative is
the same path the placement-new constructed it through. It also removes a
latent conflation: with duplicate alternative types, cast<B>() resolved to the
first index of type B rather than to the index actually asked for.
VariantStorage has two specialisations, as a standard-library variant does.
Nothing is implicitly constructed or destroyed in the storage -- Coproduct does
both explicitly via TrivialOperations -- but the destructor has to stay
*trivial* when every alternative is trivially destructible, or Coproduct loses
its own triviality.
Layout is unchanged. Compiling the old and the new header side by side gives
sizeof 16 for Coproduct<VarNumber,SymbolId> and 24 for
Coproduct<int,double,char> either way, and the in-header
is_trivially_copyable<RawCoproduct<int,int>> assertion still holds, as do
is_trivially_copyable and is_trivially_destructible for a two-POD Coproduct and
!is_trivially_destructible for Coproduct<std::vector<int>,int>.
Found via an -flto=auto build of this branch, where
vampire -sa otter Problems/SET/SET978+1.p
could not find a substitution tree child that was demonstrably present:
Top::compare, which reads the Coproduct payload, disagreed with the structure.
Putting __attribute__((optimize("no-strict-aliasing"))) on cast() fixed it, and
so did __attribute__((noinline)) alone -- the two preconditions of one
mechanism, inlining bringing the write and the read into a single body and TBAA
then being allowed to conclude they are unrelated.
Verified: ctest 103/103, and SET978+1, PUZ032-1, SYN084-1, SEU144+1, SWV019-1,
ANA019-2, GRA013+1 all answered correctly by a debug build.
Co-Authored-By: Claude Opus 5 <[email protected]>
MichaelRawson
force-pushed
the
lto-mystery
branch
from
October 11, 2026 09:33
6d23532 to
defe02c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SubstitutionTree_impl(should be cheap)SkipListsCoproduct, storing its payload in a union, not a reinterpreted byte buffer, which actually fixed the binaryA performance regression test is under way, which will also suggest how much LTO brings us, roughly.